DDB Mapper filter expressions (codegen components) #1424

ianbotsf · 2024-09-29T22:57:14Z

Issue #

Related to #472

Description of changes

The other half of #1401, this change adds codegen support for Filter expressions and runtime/codegen support for Key Condition expressions required by Query operations.

Key areas of change:

MemberCodegenBehavior has been expanded with new behaviors for expressions and behavior derivation has been spruced up to a new rules-based system
OperationRenderer now generates the logic for instantiating an expression visitor, applying it to the various operation expressions, and setting the expression literal and attributes on the low-level request
BuilderRenderer now generates DSL methods for annotated types
QueryTest is now fixed up to remove the last of the hacks necessary to run queries

Key additions:

KeyFilter and SortKeyFilter are new types to support Key Condition expressions in Query operations. KeyFilter supports partition key equality comparisons for literal values only. SortKeyFilter contains a subset of Filter methods suitable for use on sort keys (again, for literal values only).
ScanTest now exists for the Scan operation

Aside from that, a lot of small moves, renames, and refactors to deal with the new changes.

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

github-actions · 2024-09-29T23:06:39Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

0marperez · 2024-10-01T08:45:31Z

...-mapper-codegen/src/main/kotlin/aws/sdk/kotlin/hll/dynamodbmapper/codegen/model/MapperPkg.kt

+            public val Base: String = "${Hl.Base}.expressions"
+            public val Internal: String = "$Base.internal"


Question: Is there a difference here between Hl.Base and Base?

Yes there is, Base refers to the value defined on L26. Hl.Base is defined on L13

0marperez · 2024-10-01T16:58:11Z

...er/dynamodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/expressions/SortKeyFilter.kt

+    @Suppress("INAPPLICABLE_JVM_NAME")
+    @JvmName("isInRangeUShort")


Question: Is the suppression still needed even though we change the JVM name?

Unfortunately yes. Without the suppression, the compiler warns that @JvmName is inapplicable to this declaration. Without the @JvmName, the compiler warns that a platform type conflict will occur. This is a known compiler bug which should be fixed in an upcoming release.

It looks like that was fixed in 2.0.20 which is now released, we should just have to upgrade

0marperez · 2024-10-01T16:59:09Z

...egen/src/main/kotlin/aws/sdk/kotlin/hll/dynamodbmapper/codegen/operations/model/Structure.kt


            MemberCodegenBehavior.ListMapAll -> {
                val llListType = llMember.type as? TypeRef ?: error("`ListMapAll` member is required to be a TypeRef")
-                val hlListType = llListType.copy(genericArgs = listOf(TypeVar("T")), nullable = llListType.nullable)
+                val hlListType = llListType.copy(genericArgs = listOf(TypeVar("T")))


Question: Just double checking that this doesn't require nullable anymore

It never did require setting nullable. This is a copy call which carries forward all existing values unless overridden. The previous code setting nullable = llListType.nullable is redundant because that's already the value of nullable. Not sure why I wrote it that way originally but I noticed it was unnecessary when extracting nullable out to a separate val on L21.

0marperez · 2024-10-01T17:17:11Z

...n/kotlin/aws/sdk/kotlin/hll/dynamodbmapper/codegen/operations/model/MemberCodegenBehavior.kt

+    Rule("keyConditionExpression", Types.Kotlin.String, ExpressionLiteral(KeyCondition)),
+    Rule("filterExpression", Types.Kotlin.String, ExpressionLiteral(Filter)),
+
+    // TODO add support for remaining expression types


Nit:

// TODO add support for remaining expression types // Expression types

The expression types which follow the TODO are still part of the logical group // Expression literals started above.

lauzadis · 2024-10-01T21:18:31Z

hll/hll-codegen/src/main/kotlin/aws/sdk/kotlin/hll/codegen/rendering/BuilderRenderer.kt

        write("#Lvar #L: #T = null", ctx.attributes.visibility, member.name, member.type.nullable())
-        // TODO add DSL methods for structure members
+
+        ctx.info("For member $builderName.${member.name} dslInfo = $dslInfo")


nit: Is this a stray log or intentional?

It was helpful for debugging but can probably be removed now.

lauzadis · 2024-10-02T13:41:24Z

...-mapper-codegen/src/main/kotlin/aws/sdk/kotlin/hll/dynamodbmapper/codegen/model/MapperPkg.kt

+            public val Base: String = "${Hl.Base}.expressions"
+            public val Internal: String = "$Base.internal"


Yes there is, Base refers to the value defined on L26. Hl.Base is defined on L13

lauzadis · 2024-10-02T13:50:08Z

...egen/src/main/kotlin/aws/sdk/kotlin/hll/dynamodbmapper/codegen/operations/model/Structure.kt


            MemberCodegenBehavior.ListMapAll -> {
                val llListType = llMember.type as? TypeRef ?: error("`ListMapAll` member is required to be a TypeRef")
-                val hlListType = llListType.copy(genericArgs = listOf(TypeVar("T")), nullable = llListType.nullable)
+                val hlListType = llListType.copy(genericArgs = listOf(TypeVar("T")))


lauzadis · 2024-10-02T13:54:00Z

...n/kotlin/aws/sdk/kotlin/hll/dynamodbmapper/codegen/operations/rendering/OperationRenderer.kt

-    }
+    private val requestMembers = operation
+        .request
+        .also { ctx.info("For type ${it.lowLevelName}:") }


nit: "For request type", "For response type"

I omitted that because the type name already contains "request"/"response". (These are the low-level operation input/output types like ScanRequest and QueryResponse.)

lauzadis · 2024-10-02T13:55:54Z

...n/kotlin/aws/sdk/kotlin/hll/dynamodbmapper/codegen/operations/rendering/OperationRenderer.kt

-        members(MemberCodegenBehavior.Hoist) { write("#L: #T, ", name, type) }
-        write("schema: #T,", MapperTypes.Items.itemSchema("T"))
+        requestMembers(MemberCodegenBehavior.Hoist) { write("#L: #T, ", name, type) }
+        write("schema: #T", MapperTypes.Items.itemSchema("T"))


did you intend to drop the trailing comma here?

Good catch. No I shouldn't have dropped it since this parameter declaration is on its own line.

lauzadis · 2024-10-02T14:20:10Z

...er/dynamodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/expressions/SortKeyFilter.kt

+ * { sortKey eq 42 }
+ * ```
+ *
+ * This example creates an expression which checks whether an attribute named `foo` is equal to the value `42`.


checks whether an attribute named foo

foo is not defined in this example, I'm assuming it's the name of the sortKey but can it be made clearer?

Another copy-paste error. I'll rewrite the explanation and check the rest of the block for similar mistakes.

lauzadis · 2024-10-02T14:22:18Z

...er/dynamodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/expressions/SortKeyFilter.kt

+ * ```kotlin
+ * // Checks whether the value of the sort key is between 40 and 60 (inclusive)
+ * sortKey isIn 40..60
+ * ```


nit: provide an example of isBetween too

Still missing this example, but I think it can be left out based on our discussion in https://github.com/awslabs/aws-sdk-kotlin/pull/1424/files/7a1d42de3ce73bbe512ad28acca861ef9e2150b2#r1784632503

Actually, I think it is there in the latest revision. (SortKeyFilter.kt:L60-63)

I initially couldn't find it either when I searched the files on GH with my browser. But I forgot that GH hides large diffs and I had to load SortKeyFilter.kt before I found it.

Oh yeah I see it now. I did explicitly load the file but maybe was looking at a previous commit. Sorry about that!

lauzadis · 2024-10-02T14:26:16Z

...er/dynamodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/expressions/SortKeyFilter.kt

+    /**
+     * Creates a range expression for verifying the sort key is between two other expressions
+     * @param min The lower bound expression
+     * @param max The upper bound expression (inclusive)
+     */
+    public fun SortKey.isBetween(min: LiteralExpr, max: LiteralExpr): SortKeyExpr
+
+    /**
+     * Creates a range expression for verifying the sort key is between two other expressions
+     * @param min The lower bound value
+     * @param max The upper bound value (inclusive)
+     */
+    public fun SortKey.isBetween(min: ByteArray, max: ByteArray): SortKeyExpr =
+        isBetween(LiteralExpr(min), LiteralExpr(max))


question: this is missing support for isBetween with strings, is that intentional?

Excellent question!

Under the hood, both isBetween and isIn(ClosedRange<*>) get converted into BETWEEN expressions in DynamoDB's expression syntax. (A DynamoDB IN operator also exists but it's only used for checking if a member is present in a set/list, which we make available via isIn(Collection<*>).)

I made the decision both in Filter and SortKeyFilter to use different names for different argument types to closely mirror familiar Kotlin idioms. My reasoning was that, in idiomatic Kotlin, one would typically perform a range check with the in operator (something like if (foo in 40..60)). Unfortunately, Kotlin's ranges only work on Comparable types and ByteArray doesn't implement that interface. There's no way to use in to do something like foo in someByteArray..someOtherByteArray—that fails to compile. Neither would there be some way to reference attribute paths in an expression (e.g., foo in attr("bar")..attr("baz")) since those aren't comparable either.

So I needed a two-arg version of this check for LiteralExpr and ByteArray accepting a min and max. The name "in" no longer seemed appropriate since Kotlin's in is a binary operator and this is really a trinary operation. Kotlin has no good equivalent so I decided that "between" made more sense for the two-arg methods.

Now, each of the ClosedRange/single-arg methods could be written as isBetween/min+max methods but it seemed unnecessary and I'd rather hear demand for that than add it preemptively. But the inverse is not true: none of the existing isBetween/min+max methods can be rewritten as a ClosedRange/single-arg method.

OK, now that the explanations and justifications are over, the question is "does this make sense?". Do these choices make the DSL easier or harder for users to understand/use?

Discussed offline and decided the current naming schema makes sense for now.

lauzadis · 2024-10-02T14:28:59Z

...er/dynamodb-mapper/common/src/aws/sdk/kotlin/hll/dynamodbmapper/expressions/SortKeyFilter.kt

+    /**
+     * Creates a range expression for verifying the sort key is in the given range
+     * @param range The range to check
+     */
+    @Suppress("INAPPLICABLE_JVM_NAME")
+    @JvmName("isInRangeString")
+    public infix fun SortKey.isIn(range: ClosedRange<String>): SortKeyExpr =
+        isBetween(LiteralExpr(range.start), LiteralExpr(range.endInclusive))


question: Do we want to support isIn for binary types?

Longer answer above but ClosedRange requires Comparable type arguments and ByteArray is not comparable.

lauzadis · 2024-10-02T14:52:28Z

hll/hll-codegen/src/main/kotlin/aws/sdk/kotlin/hll/codegen/model/Member.kt

@@ -30,3 +40,10 @@ public data class Member(val name: String, val type: Type, val mutable: Boolean
        }
    }
 }
+
+/**
+ * Gets the low-level [Structure] equivalent for this high-level structure


nit: Gets the low-level [Member] equivalent for this high-level Member

Yep, copy-paste error.

…ing, add runtime check for acceptable partition key value types, remove unused function, re-add accidentally-removed trailing comma from codegen

github-actions · 2024-10-02T21:53:22Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

sonarcloud · 2024-10-03T15:48:35Z

Quality Gate passed

Issues
18 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.1% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-10-03T15:56:29Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK

DDB mapper filter expressions codegen

7a1d42d

ianbotsf requested a review from a team as a code owner September 29, 2024 22:57

0marperez approved these changes Oct 1, 2024

View reviewed changes

lauzadis reviewed Oct 2, 2024

View reviewed changes

address PR feedback: add/cleanup KDocs, remove unnecessary debug logg…

f3929f5

…ing, add runtime check for acceptable partition key value types, remove unused function, re-add accidentally-removed trailing comma from codegen

lauzadis approved these changes Oct 3, 2024

View reviewed changes

Merge branch 'feat-ddb-mapper' into ddbmapper-filter-expressions-codegen

887b6cb

ianbotsf merged commit e080cea into feat-ddb-mapper Oct 3, 2024
11 checks passed

ianbotsf deleted the ddbmapper-filter-expressions-codegen branch October 3, 2024 16:16

		public val Base: String = "${Hl.Base}.expressions"
		public val Internal: String = "$Base.internal"

		@Suppress("INAPPLICABLE_JVM_NAME")
		@JvmName("isInRangeUShort")

DDB Mapper filter expressions (codegen components) #1424

DDB Mapper filter expressions (codegen components) #1424

Conversation

ianbotsf commented Sep 29, 2024

Issue #

Description of changes

github-actions bot commented Sep 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 2, 2024

sonarcloud bot commented Oct 3, 2024

Quality Gate passed

github-actions bot commented Oct 3, 2024