Support inserting into table having CHECK constraint in Delta Lake #15396

ebyhr · 2022-12-14T11:42:39Z

Description

Supported expressions are intentionally limited in this PR.
~~The 1st commit came from #14964. Please leave review comments for the 1st commit in #14964.~~

Release notes

(x) Release notes are required, with the following suggested text:

# Delta Lake
* Support inserting into tables having `CHECK` constraints. ({issue}`15396`)

findepi · 2022-12-16T09:40:56Z

plugin/trino-delta-lake/src/main/antlr4/io/trino/plugin/deltalake/expression/SparkExpression.g4

+number
+    : MINUS? INTEGER_VALUE  #integerLiteral


what about decimals and scientific notation?

Left a TODO comment. Let me know if we should support them in v1.

plugin/trino-delta-lake/src/main/antlr4/io/trino/plugin/deltalake/expression/SparkExpression.g4

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/AstVisitor.java

...-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/SparkExpressionConverter.java

...rino-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/ComparisonExpression.java

...rino-delta-lake/src/test/java/io/trino/plugin/deltalake/expression/TestSparkExpressions.java

findepi · 2022-12-16T10:39:30Z

What about LIKE and function calls, like substring?

ebyhr · 2022-12-19T07:31:52Z

What about LIKE and function calls, like substring?

@findepi Left a TODO comment in g4 file.

ebyhr · 2022-12-19T09:38:58Z

CI hit #13199

findepi · 2022-12-19T09:51:33Z

What about LIKE and function calls, like substring?

@findepi Left a TODO comment in g4 file.

Sounds good, unless mini-grammar is not the way to go at all.

Can we have alternative approach PR where we explore Coral-based translation and see how hard it would be to get it working?

ebyhr · 2022-12-19T12:49:51Z

@findepi Coral doesn't support parsing Spark SQL as far as I know. Are you suggesting to try Hive translation anyway?

ebyhr · 2022-12-19T21:53:52Z

Rebased on upstream to resolve conflicts.

ebyhr · 2023-01-25T02:33:06Z

Rebased on upstream to include engine and SPI change.

findepi · 2023-01-25T16:35:48Z

@findepi Coral doesn't support parsing Spark SQL as far as I know.

let's add this as a commit message comment, covering alternatives we considered and rejected

Are you suggesting to try Hive translation anyway?

no

Rebased on upstream to include engine and SPI change.

feel free to squash the fixups

ebyhr · 2023-01-25T22:31:48Z

@findepi Squashed commits and updated the commit message.

findepi

Reviewed Parsing (up to and excluding "SparkExpressionConverter.java").

I think it would be a good idea to have explicit parser tests for String literals (both positive and negative)

plugin/trino-delta-lake/pom.xml

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/Identifier.java

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/Literal.java

...rino-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/ComparisonExpression.java

...no-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/SparkExpressionBuilder.java

ebyhr · 2023-01-30T09:16:22Z

Addressed comments.

CI hit #13199 and #14441

findepi · 2023-02-03T12:45:20Z

...n/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/expression/TestStringLiteral.java

+    public void testUnsupportedStringLiteral()
+    {
+        assertParseFailure("r'raw literal'");
+        assertParseFailure("r\"'\\n' represents dnewline character.\"");


dnewline -> newline

what's unsupported here? the r prefix?
for assertParseFailure can you assert on the exact exception message being thrown?

findepi · 2023-02-03T12:49:45Z

...in/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/SparkExpressions.java

+            return (SparkExpression) invokeParser(expressionPattern, SparkExpressionParser::standaloneExpression);
+        }
+        catch (Exception e) {
+            throw new ParsingException("Cannot parse Spark expression: " + expressionPattern);


throw new ParsingException("Cannot parse Spark expression [%s]: %s".formatted(expressionPattern, firstNonNull(e.getMessage(), e)), e);

findepi · 2023-02-03T12:49:58Z

...in/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/SparkExpressions.java

@@ -51,9 +52,15 @@ public static String toTrinoExpression(String sparkExpression)
        }
    }

-    private static SparkExpression createExpression(String expressionPattern)
+    @VisibleForTesting
+    static SparkExpression createExpression(String expressionPattern)


expressionPattern -> expression

(it's definitely not a pattern)

findepi · 2023-02-03T13:23:39Z

...-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/SparkExpressionConverter.java

+        return new Formatter().process(expression, null);
+    }
+
+    public static class Formatter


findepi · 2023-02-03T13:31:31Z

...-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/SparkExpressionConverter.java

+        @Override
+        protected String visitLogicalExpression(LogicalExpression node, Void context)
+        {
+            return "(%s %s %s)".formatted(process(node.getLeft(), context), node.getOperator().toString(), process(node.getRight(), context));


You may be able to avoid some braces on the output by glueing together series of ANDs or ORs

@Override protected String visitLogicalExpression(LogicalExpression node, Void context) { return stream(flatten(node)) .map(expression -> process(expression, context)) .collect(Collectors.joining(node.getOperator().toString(), "(", ")")); } private Iterable<SparkExpression> flatten(LogicalExpression root) { return Traverser.<SparkExpression>forTree(node -> { if (node instanceof LogicalExpression logicalExpression && logicalExpression.getOperator() == root.getOperator()) { return ImmutableList.of(logicalExpression.getLeft(), logicalExpression.getRight()); } return ImmutableList.of(); }) .depthFirstPreOrder(root); }

not sure it's worth it

It causes a stack overflow. Let me handle in follow-up.

findepi · 2023-02-03T13:32:51Z

...-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/SparkExpressionConverter.java

+        @Override
+        protected String visitLongLiteral(LongLiteral node, Void context)
+        {
+            return String.valueOf(node.getValue());


i'd prefer Long.toString emphasizing the value is known to be long (and thus safe to output as trino exrepssion)

similarly, Boolean.toString above

findepi · 2023-02-03T13:34:32Z

...in/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/SparkExpressions.java

+            return new SparkExpressionBuilder().visit(tree);
+        }
+        catch (StackOverflowError e) {
+            throw new IllegalArgumentException("expression pattern is too large (stack overflow while parsing)");


rm "pattern"

findepi · 2023-02-03T13:36:06Z

...n/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/expression/TestStringLiteral.java

+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
+import static org.testng.Assert.assertEquals;
+
+public class TestStringLiteral


This is a parser test, not a StringLiteral test

TestSparkExpressions (or however the parsing class gets renamed)

findepi · 2023-02-03T13:38:39Z

...in/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/expression/SparkExpressions.java

+
+import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;
+
+public final class SparkExpressions


Can call this class SparkExpressionParser?

the antlr generated class would want to be called eg SparkExpressionBaseParser
(just rename grammar file and update grammar SparkExpression; line)

findepi · 2023-02-03T13:39:58Z

...rc/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCheckConstraintCompatibility.java

+            assertThatThrownBy(() -> onDelta().executeQuery("INSERT INTO default." + tableName + " VALUES (" + invalidInput + ")"))
+                    .hasMessageMatching("(?s).* CHECK constraint .* violated by row with values.*");
+            assertThatThrownBy(() -> onTrino().executeQuery("INSERT INTO delta.default." + tableName + " VALUES (" + invalidInput + ")"))
+                    .hasMessageContaining("Check constraint violation");


what is the exact failure message?

The format is "Check constraint violation: " + constraint (constraint = Trino expression). It's tested in TestCheckConstraint.

Use ANTLR approach because LinkedIn Coral doesn't support translating Spark SQL yet. Additionally, extract relevant tests from TestDeltaLakeCheckConstraintsCompatibility.

ebyhr · 2023-02-06T10:11:58Z

CI hit #14441

cla-bot bot added the cla-signed label Dec 14, 2022

github-actions bot added the tests:hive label Dec 14, 2022

findepi reviewed Dec 16, 2022

View reviewed changes

ebyhr marked this pull request as ready for review December 19, 2022 01:42

martint force-pushed the master branch from 40dbb4f to 0d73d10 Compare December 19, 2022 20:15

ebyhr force-pushed the ebi/delta-spark-check-constraint-parser branch from b4fa35c to 31efb02 Compare December 19, 2022 21:53

ebyhr force-pushed the ebi/delta-spark-check-constraint-parser branch from 31efb02 to 596b07d Compare January 25, 2023 02:32

ebyhr force-pushed the ebi/delta-spark-check-constraint-parser branch from 596b07d to f792ef3 Compare January 25, 2023 22:31

ebyhr self-assigned this Jan 26, 2023

findepi reviewed Jan 27, 2023

View reviewed changes

ebyhr force-pushed the ebi/delta-spark-check-constraint-parser branch from f792ef3 to caaa9a3 Compare January 30, 2023 05:34

findepi reviewed Feb 3, 2023

View reviewed changes

findepi approved these changes Feb 3, 2023

View reviewed changes

Support inserting into table having CHECK constraint in Delta Lake

21734a0

Use ANTLR approach because LinkedIn Coral doesn't support translating Spark SQL yet. Additionally, extract relevant tests from TestDeltaLakeCheckConstraintsCompatibility.

ebyhr force-pushed the ebi/delta-spark-check-constraint-parser branch from caaa9a3 to 21734a0 Compare February 6, 2023 01:38

ebyhr merged commit 60a0ee9 into master Feb 6, 2023

ebyhr deleted the ebi/delta-spark-check-constraint-parser branch February 6, 2023 11:10

ebyhr mentioned this pull request Feb 6, 2023

Release notes for 407 #15854

Closed

github-actions bot added this to the 407 milestone Feb 6, 2023

colebow mentioned this pull request Feb 10, 2023

Add Trino 407 release notes #15919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support inserting into table having CHECK constraint in Delta Lake #15396

Support inserting into table having CHECK constraint in Delta Lake #15396

ebyhr commented Dec 14, 2022 •

edited

Loading

findepi Dec 16, 2022

ebyhr Dec 19, 2022

findepi commented Dec 16, 2022

ebyhr commented Dec 19, 2022 •

edited

Loading

ebyhr commented Dec 19, 2022

findepi commented Dec 19, 2022

ebyhr commented Dec 19, 2022

ebyhr commented Dec 19, 2022

ebyhr commented Jan 25, 2023

findepi commented Jan 25, 2023

ebyhr commented Jan 25, 2023

findepi left a comment

ebyhr commented Jan 30, 2023

findepi Feb 3, 2023

findepi Feb 3, 2023 •

edited

Loading

findepi Feb 3, 2023

findepi Feb 3, 2023

findepi Feb 3, 2023

findepi Feb 3, 2023

ebyhr Feb 6, 2023

findepi Feb 3, 2023

findepi Feb 3, 2023

findepi Feb 3, 2023

findepi Feb 3, 2023

findepi Feb 3, 2023

findepi Feb 3, 2023

ebyhr Feb 6, 2023

ebyhr commented Feb 6, 2023


		import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;

		public final class SparkExpressions

Support inserting into table having CHECK constraint in Delta Lake #15396

Support inserting into table having CHECK constraint in Delta Lake #15396

Conversation

ebyhr commented Dec 14, 2022 • edited Loading

Description

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Dec 16, 2022

ebyhr commented Dec 19, 2022 • edited Loading

ebyhr commented Dec 19, 2022

findepi commented Dec 19, 2022

ebyhr commented Dec 19, 2022

ebyhr commented Dec 19, 2022

ebyhr commented Jan 25, 2023

findepi commented Jan 25, 2023

ebyhr commented Jan 25, 2023

findepi left a comment

Choose a reason for hiding this comment

ebyhr commented Jan 30, 2023

Choose a reason for hiding this comment

findepi Feb 3, 2023 • edited Loading

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

ebyhr commented Feb 6, 2023

ebyhr commented Dec 14, 2022 •

edited

Loading

ebyhr commented Dec 19, 2022 •

edited

Loading

findepi Feb 3, 2023 •

edited

Loading