feat: add functions (sort, sort_by, max_by, min_by, map) to JMESPath visitor #969

0marperez · 2023-10-03T19:30:36Z

Issue #

closes #596

Description of changes

-Added sort to JMESPath visitor
-Added sort_by to JMESPath visitor
-Added max_by to JMESPath visitor
-Added min_by to JMESPath visitor
-Added map to JMESPath visitor

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

lauzadis · 2023-10-04T13:56:40Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

-        val ident = addTempVar("comparison", codegen)
-        return VisitedExpression(ident)
+        val unSafeComparatorValue = addTempVar("unSafeComparator", codegen)
+        val safeComparatorValue = addTempVar("safeComparator", "if ($unSafeComparatorValue == null) null else $unSafeComparatorValue ${expression.comparator} 0")


you can avoid a null check by doing this:

$unSafeComparatorValue ?: $unSafeComparatorValue ${expression.comparator} 0

lauzadis · 2023-10-04T14:11:19Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+    ): VisitedExpression {
+        val (argIndex, expressionIndex) = if (invertedArgs) 1 to 0 else 0 to 1
+
+        codegenReq(expression.arguments.size == 2) { "Unexpected number of arguments to $this" }


Let's print the number of expected arguments and the number of actual arguments received

lauzadis · 2023-10-04T14:12:16Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+        function: (JmespathExpression, VisitedExpression) -> String,
+        invertedArgs: Boolean = false,
+    ): VisitedExpression {
+        val (argIndex, expressionIndex) = if (invertedArgs) 1 to 0 else 0 to 1


Can you explain why map has inverted arguments? I'm wondering if there is a "nicer" way to handle this?

The JMESPath map function has the arguments inverted, you can compare it with max_by and the rest

lauzadis · 2023-10-04T14:13:23Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+            if (stringOrNumberCheck) {
+                write("if ($expressionValue as Any !is Number && $expressionValue as Any !is String) throw Exception(\"Result of applying expression should be string or number\")")
+            }


Same comment about invertedArgs here, why is this needed for map only?

The map expression can result in any type but for the rest (max_by, min_by, etc..) the expression should result in a string or a number.

lauzadis · 2023-10-04T14:14:00Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+    ): String {
+        val argName = arg.identifier
+        val result = bestTempVarName(resultName)
+        writer.withBlock("val $result = $argName?.$operation{", "}") {


nit: space between $operation and {

lauzadis · 2023-10-04T14:14:51Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+        writer.withBlock("val $result = $argName?.$operation{", "}") {
+            val expressionValue = addTempVar("expression", subfieldCodegen((expression as ExpressionTypeExpression).expression as FieldExpression, "it"))
+            if (stringOrNumberCheck) {
+                write("if ($expressionValue as Any !is Number && $expressionValue as Any !is String) throw Exception(\"Result of applying expression should be string or number\")")


Can we throw a more descriptive exception type than the base Exception?

lauzadis · 2023-10-04T14:18:44Z

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/util/JMESPath.kt

+@InternalApi
+@JvmName("StringJmespathSort")
+public fun List<String>.jmespathSort(): List<String> = this.sorted()
+
+@InternalApi
+@JvmName("ShortJmespathSort")
+public fun List<Short>.jmespathSort(): List<Short> = this.sorted()
+
+@InternalApi
+@JvmName("IntJmespathSort")
+public fun List<Int>.jmespathSort(): List<Int> = this.sorted()
+
+@InternalApi
+@JvmName("FloatJmespathSort")
+public fun List<Float>.jmespathSort(): List<Float> = this.sorted()
+
+@InternalApi
+@JvmName("LongJmespathSort")
+public fun List<Long>.jmespathSort(): List<Long> = this.sorted()
+
+@InternalApi
+@JvmName("DoubleJmespathSort")
+public fun List<Double>.jmespathSort(): List<Double> = this.sorted()


Can this be made generic? You would lose the @JvmName annotation but also remove redundant functions.
If the annotation is not required, I'd try to make these generic for any T

lauzadis · 2023-10-04T14:20:56Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

@@ -548,6 +585,37 @@ class KotlinJmespathExpressionVisitor(
        return notNull
    }

+    private fun mapFunctionLogic(


I'd call this applyFunction, since mapFunctionLogic could be confused for the actual map function, and I think the Logic suffix is unnecessary

Would it make sense to make this an extension function of JmespathExpression? Then you can use it like this

private fun JmespathExpression.applyFunction( ... ) { ... } expression.applyFunction(arg, "sorted", "sortedBy")

aajtodd · 2023-10-04T13:51:39Z

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/util/JMESPath.kt

+
+@InternalApi
+@JvmName("StringJmespathSort")
+public fun List<String>.jmespathSort(): List<String> = this.sorted()


correctness: Why do we even need these? Can't we just call sorted() directly?

I added them because JMESPath's sort can only be applied to numbers and strings. I think it makes sense to check the type and then throw an exception explaining that if it's the wrong type of list

Again I don't think we want to be responsible for runtime checking things that could be caught by model validation. These just inflate the size of the runtime core artifact with no real value. I think we should remove them and just call sorted directly which is already defined with generics

aajtodd · 2023-10-04T13:54:00Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

@@ -159,14 +169,16 @@ class KotlinJmespathExpressionVisitor(
                append("if ($isNullExpr) null else ")
            }

-            val unSafeComparatorExpr = "compareTo(${right.identifier}) ${expression.comparator} 0"
+            val unSafeComparatorExpr = "compareTo(${right.identifier})"


nit: unsafe is one word -> unsafeComparatorExpr

Also this generates code like:

val unSafeComparator = if (integer == null) null else integer?.compareTo(number) val safeComparator = if (unSafeComparator == null) null else unSafeComparator == 0 safeComparator == true

Where you're just taking the null value if it's null otherwise comparing it to the expected value. You can compare null directly though

val comparator = integer == 0

Another example:

val lists = it.lists val strings = lists?.strings val length = strings?.length ?: 0 val number = 0.0 val unSafeComparator = if (length == null) null else length.compareTo(number) val safeComparator = if (unSafeComparator == null) null else unSafeComparator > 0 safeComparator == true

it can be condensed to:

val safeComparator = length > 0

If the left expression of the comparision is nullable it can be changed to:

val safeComparator = length?.let { it > 0 }

Which will yield Boolean? as the result.

aajtodd · 2023-10-04T13:56:58Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+            arg.dotFunction(expression, "jmespathSort()")
+        }
+
+        "sort_by" -> mappingFunction(expression, "sortBy", this::sortBy)


style: You can drop this -> ::sortBy

aajtodd · 2023-10-04T14:22:37Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+        writer.withBlock("val $result = $argName?.$operation{", "}") {
+            val expressionValue = addTempVar("expression", subfieldCodegen((expression as ExpressionTypeExpression).expression as FieldExpression, "it"))
+            if (stringOrNumberCheck) {
+                write("if ($expressionValue as Any !is Number && $expressionValue as Any !is String) throw Exception(\"Result of applying expression should be string or number\")")


correctness:

This check is wrong:

val sorted = structs?.sortedBy{ val expression = it?.integer if (expression as Any !is Number && expression as Any !is String) throw Exception("Result of applying expression should be string or number") expression }

expression is of type Any? so as Any is incorrect. Expression is likely never going to be null anyway though so I guess in practice it will be Any at runtime. If we can we should generate the code as it.integer not it? here since .sortedBy will only run if structs isn't null in the first place (due to ?).

Backing up for a second, why do we even need to generate the stringOrNumber check in the first place? I see in the JMESPath spec:

If the result of evaluating the expr against the current array element results in type other than a number or a string, an invalid-type error will occur.

But why do we need to worry about this? Seems like this would be an invalid model most likely.

But why do we need to worry about this? Seems like this would be an invalid model most likely.

The expression resulting in anything other than a string or number is not being checked for at the model level. I think it makes sense to have that check. Someone could add an expression that results in something else and it would fail without an exception explaining why

But that would be a modeling issue, I don't think we need to add this check here. Shouldn't we be able to check this at codegen time if we track the expression types correctly?

Also Smithy is doing some validation on the JMESPath expressions in the model: https://github.com/smithy-lang/smithy/blob/main/smithy-jmespath/src/main/java/software/amazon/smithy/jmespath/TypeChecker.java

I don't think we need to add runtime validations here, this is the kind of stuff that belongs as an error in the model such that it never can even get into the hands of a customer.

aajtodd · 2023-10-04T14:26:38Z

...n/software/amazon/smithy/kotlin/codegen/rendering/waiters/KotlinJmespathExpressionVisitor.kt

+    ): String {
+        val argName = arg.identifier
+        val result = bestTempVarName(resultName)
+        writer.withBlock("val $result = $argName?.$operation{", "}") {


nit: add space between $operation and opening brace {

sonarqubecloud · 2023-10-05T18:57:09Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
6.9% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Added functions

7e06e45

0marperez changed the title ~~feat: Added functions~~ feat: add functions (sort, sort_by, max_by, min_by, map) to JMESPath visitor Oct 3, 2023

0marperez added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Oct 3, 2023

0marperez marked this pull request as ready for review October 3, 2023 19:50

0marperez requested a review from a team as a code owner October 3, 2023 19:50

lauzadis reviewed Oct 4, 2023

View reviewed changes

aajtodd reviewed Oct 4, 2023

View reviewed changes

0marperez added 2 commits October 5, 2023 14:46

Address PR feedback

10c8154

Removed deleted runtime import

8bb010a

aajtodd approved these changes Oct 9, 2023

View reviewed changes

ianbotsf approved these changes Oct 10, 2023

View reviewed changes

0marperez merged commit 031b50c into main Oct 10, 2023
4 checks passed

0marperez deleted the jmes-path-functions branch October 10, 2023 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add functions (sort, sort_by, max_by, min_by, map) to JMESPath visitor #969

feat: add functions (sort, sort_by, max_by, min_by, map) to JMESPath visitor #969

0marperez commented Oct 3, 2023 •

edited

Loading

lauzadis Oct 4, 2023

lauzadis Oct 4, 2023

lauzadis Oct 4, 2023

0marperez Oct 5, 2023

lauzadis Oct 4, 2023

0marperez Oct 5, 2023

lauzadis Oct 4, 2023

lauzadis Oct 4, 2023

lauzadis Oct 4, 2023

lauzadis Oct 4, 2023

lauzadis Oct 4, 2023

aajtodd Oct 4, 2023

0marperez Oct 5, 2023

aajtodd Oct 5, 2023

aajtodd Oct 4, 2023

aajtodd Oct 4, 2023

aajtodd Oct 4, 2023

aajtodd Oct 4, 2023

0marperez Oct 5, 2023

aajtodd Oct 5, 2023

aajtodd Oct 5, 2023

aajtodd Oct 4, 2023

sonarqubecloud bot commented Oct 5, 2023

feat: add functions (sort, sort_by, max_by, min_by, map) to JMESPath visitor #969

feat: add functions (sort, sort_by, max_by, min_by, map) to JMESPath visitor #969

Conversation

0marperez commented Oct 3, 2023 • edited Loading

Issue #

Description of changes

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

sonarqubecloud bot commented Oct 5, 2023

0marperez commented Oct 3, 2023 •

edited

Loading