Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DDB Mapper filter expressions (runtime components) #1401

Merged
merged 4 commits into from
Sep 17, 2024

Conversation

ianbotsf
Copy link
Contributor

@ianbotsf ianbotsf commented Sep 9, 2024

Issue #

Related to #76

Description of changes

This change adds low-level filter expressions support to the runtime, although nothing uses them yet. The filter DSL is defined in the Filter interface and will eventually (in the next PR) be part of a Lambda signature in a codegenned member for operation requests.

Something like this:

// Generated code
interface QueryRequest<T> {
    // Property syntax
    val filter: Filter.() -> BooleanExpr
    
    // DSL method syntax
    fun filter(block: Filter.() -> BooleanExpr)

    // …all other Query members
}

Callers will then use this when building their query structures to specify their filter expression (if desired):

// User code
table.query {
    filter {
        and(
            attr("productName") eq "Nimbus 2000",
            attr("condition") isIn setOf("New", "Refurbished"),
            attr("price") lt 500.00,
        )
    }

    // …set other Query members
}

In addition to the normal things, I could especially use feedback on the DSL—names, conventions, types, etc. If it's incomprehensible to you then it'll probably be even worse for users!

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ianbotsf ianbotsf requested a review from a team as a code owner September 9, 2024 22:15
Copy link

github-actions bot commented Sep 9, 2024

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

Copy link
Contributor Author

@ianbotsf ianbotsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Captured some notes from a live code review with the team today. More comments/reviews to follow.

Comment on lines +201 to +205
/**
* Creates an equality expression for verifying two expressions are equal to each other
* @param value The other value in the comparison
*/
public infix fun Expression.eq(value: Boolean?): BooleanExpr = eq(LiteralExpr(value))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

live review: Can we collapse KDocs for multiple methods into a single shared instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out we can but it only affects Dokka-generated documentation. IntelliJ will only find KDocs directly attached to each overload. I'm going to leave the documentation on overloads for the time being.

Comment on lines +652 to +659
/**
* Creates a range expression for verifying this expression is in the given range
* @param range The range to check
*/
@Suppress("INAPPLICABLE_JVM_NAME")
@JvmName("isInRangeNumber")
public infix fun <N> AttributePath.isIn(range: ClosedRange<N>): BooleanExpr where N : Number, N : Comparable<N> =
isBetween(LiteralExpr(range.start), LiteralExpr(range.endInclusive))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

live review: Should we support open ranges now or in the future? Investigate and/or leave a TODO

Comment on lines 162 to 169
* Several additional filter expressions are possible via the following methods/properties:
*
* * [contains] — checks if a string/list attribute value contains the given value
* * [exists] — checks if _any value_ (including `null`) exists for an attribute
* * [notExists] — checks if no value is present for an attribute (i.e., the attribute is "undefined" for an item)
* * [isOfType] — checks if an attribute value is of the given type
* * [size] — gets the size of an attribute (e.g., number of elements in list/map/set, the length of a string, etc.)
* * [startsWith] — checks if a string attribute value starts with the given value
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

live review: Here and elsewhere when referring to Kotlin-ized names of DDB primitives, mention the underlying primitive to help users get up to speed, transition from another mapper, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add a few examples here.

include(":hll:dynamodb-mapper:tests:dynamodb-mapper-schema-generator-plugin-test")
// include(":hll:dynamodb-mapper:tests:dynamodb-mapper-schema-generator-plugin-test")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

live review: accidental inclusion, remove

Comment on lines +15 to +31
/**
* Represents any kind of expression. This is an abstract top-level interface and describes no details about an
* expression on its own. Expressions may be of various specific types (e.g., [AttributePath], [ComparisonExpr],
* [AndExpr], etc.) each of which have specific data detailing the expression.
*
* [Expression] and its derivatives support the [visitor design pattern](https://en.wikipedia.org/wiki/Visitor_pattern)
* by way of an [accept] method.
*/
public sealed interface Expression {
/**
* Accepts a visitor that is traversing an expression tree by dispatching to a subtype implementation. Subtype
* implementations MUST call the [ExpressionVisitor.visit] overload for their concrete type (effectively forming a
* [double dispatch](https://en.wikipedia.org/wiki/Double_dispatch)) and MUST return the resulting value.
* @param visitor The [ExpressionVisitor] that is traversing an expression
*/
public fun <T> accept(visitor: ExpressionVisitor<T>): T
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

live review: split out hierarchy into different smaller files

Copy link
Member

@lauzadis lauzadis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong comments / suggestions outside of what we covered in the live review!

String("S"),

/**
* String set data type, denoted in DynamoDB as `S`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: SS

* * The value of attribute `bar` is less than the value of `baz` **–and–** the value of `baz` is greater/equal to `42`
* * The value of attribute `qux` is not one of `"ready"`, `"steady"`, or `"go"`
*
* This is roughly equivalent to the Kotlin syntax:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

roughly equivalent -> logically equivalent?

GREATER_THAN(">"),

/**
* A greater-than-or-equal-to comparison, equivalent to `>=` in Kotlin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "in Kotlin and DynamoDB"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opinion: I feel like the implementations don't need to repeat the KDocs of what they are implementing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what you mean? This ScalarFunc enum doesn't implement a particular interface...it identifies a particular low-level function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is referring to the entire ExpressionImpls.kt file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh jeez, I can't read. 🤦‍♂️

Yeah, I can remove the KDocs for the internal implementations. Still calibrating when it's helpful to leave KDocs on non-public stuff vs when it's just noise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was thinking in this case they are repeated on the public interfaces, so the internal implementations don't need them

…heir own files, revert accidental settings.gradle.kts change, clean up various KDocs
Copy link

sonarcloud bot commented Sep 12, 2024

Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK

@ianbotsf ianbotsf merged commit 4d71a04 into feat-ddb-mapper Sep 17, 2024
11 checks passed
@ianbotsf ianbotsf deleted the ddbmapper-filter-expressions-runtime branch September 17, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants