Use attachValues in SQL #11259

reuvenlax · 2020-03-29T23:23:57Z

Use the more efficient attachValues when building rows in SQL. addValues does a deep copy of the parameter, while addValue does not. We have also recently made API improvements that makes addValues slightly less efficient, so this allows SQL to bypass that.

We are not using attachValues on any of the IOs. This is strictly for internal rows generated by SQL.

reuvenlax · 2020-03-29T23:33:58Z

run sql postcommit

alexvanboxel · 2020-03-30T05:31:03Z

Use the more efficient attachValues when building rows in SQL. addValues does a deep copy of the parameter, while addValue does not. We have also recently made API improvements that makes addValues slightly less efficient, so this allows SQL to bypass that.

We are not using attachValues on any of the IOs. This is strictly for internal rows generated by SQL.

I've changed the description from while attachValues does not too while addValue does not. (typo)

apilloud

Before this change the validation in addValue would ensure output types were correct, so I'm not sure that our tests explicitly check for that failure case. Can you verify the tests still catch regressions where incorrect types are produced by SQL?

LGTM

apilloud · 2020-03-30T16:09:26Z

Actually it looks like this completely broke BeamCalcRel. You should fix that too.

reuvenlax · 2020-04-01T23:07:58Z

run sql postcommit

apilloud · 2020-04-01T23:12:02Z

FYI, I was going through our internal test failures and found a bunch that will probably be much more difficult to debug after this change. I included a sample of the current failures below, it would be nice to have a way to turn SchemaVerification on for testing.

Caused by: java.lang.IllegalArgumentException: FieldType{typeName=INT64, nullable=false, logicalType=null, collectionElementType=null, mapKeyType=null, mapValueType=null, rowSchema=null, metadata={}} is not nullable in Array field $arrayx
	at org.apache.beam.sdk.values.SchemaVerification.verifyArray(SchemaVerification.java:101)
	at org.apache.beam.sdk.values.SchemaVerification.verifyFieldValue(SchemaVerification.java:65)
	at org.apache.beam.sdk.values.SchemaVerification.verifyRowValues(SchemaVerification.java:57)
	at org.apache.beam.sdk.values.Row$Builder.build(Row.java:673)
	at org.apache.beam.sdk.extensions.sql.zetasql.BeamZetaSqlCalcRel$CalcFn.processElement(BeamZetaSqlCalcRel.java:195)

reuvenlax · 2020-04-02T17:00:03Z

@apilloud I added a pipeline option that can be used to enable more verbose Row checking. Dataflow can enable this on its internal tests.

apilloud · 2020-04-02T23:45:28Z

LGTM

TheNeuralBit · 2020-04-03T20:11:45Z

Run SQL PostCommit

TheNeuralBit

It's concerning that none of the checks caught this error. Is SQL PreCommit not building this code? Hopefully SQL PostCommit catches it

TheNeuralBit · 2020-04-03T20:09:11Z

...nsions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamAggregationRel.java

+ Row row =
+ verifyRowValues
+ ? Row.withSchema(outputSchema).addValues(fieldValues).build()
+ : Row.withSchema(outputSchema).attachValues(fieldValues);


I think this is missing a .build()

pr/10883 changed attachValues() to return a Row, so build() not needed.

TheNeuralBit · 2020-04-03T20:14:15Z

...sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/BeamZetaSqlCalcRel.java

+ Row outputRow =
+ verifyRowValues
+ ? Row.withSchema(outputSchema).addValues(values).build()
+ : Row.withSchema(outputSchema).attachValues(values);


TheNeuralBit · 2020-04-03T20:14:22Z

...sions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSqlUtils.java

+ Row row =
+ verifyValues
+ ? Row.withSchema(schema).addValues(objects).build()
+ : Row.withSchema(schema).attachValues(objects);


TheNeuralBit · 2020-04-03T20:21:28Z

PostCommit doesn't get it either..

TheNeuralBit

Ah nevermind, the tests are right and I am wrong. This API changed in #10883

Reuven Lax added 2 commits March 29, 2020 15:10

foo

064f88c

replace addValues with attachValues in SQL

348e746

probot-autolabeler bot added extensions java sql labels Mar 29, 2020

fix comments

4dd5391

reuvenlax requested a review from apilloud March 29, 2020 23:24

apilloud approved these changes Mar 30, 2020

View reviewed changes

fix BeamCalcRel

fb15f8a

Reuven Lax added 3 commits April 1, 2020 17:10

foo

7f806d2

fix failures and add pipeline option to control

82973bd

fix return

0155354

TheNeuralBit requested changes Apr 3, 2020

View reviewed changes

TheNeuralBit approved these changes Apr 3, 2020

View reviewed changes

reuvenlax merged commit ed5a358 into apache:master Apr 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use attachValues in SQL #11259

Use attachValues in SQL #11259

reuvenlax commented Mar 29, 2020 •

edited by alexvanboxel

Loading

reuvenlax commented Mar 29, 2020

alexvanboxel commented Mar 30, 2020 •

edited

Loading

apilloud left a comment

apilloud commented Mar 30, 2020

reuvenlax commented Apr 1, 2020

apilloud commented Apr 1, 2020

reuvenlax commented Apr 2, 2020

apilloud commented Apr 2, 2020

TheNeuralBit commented Apr 3, 2020

TheNeuralBit left a comment

TheNeuralBit Apr 3, 2020

reuvenlax Apr 3, 2020

TheNeuralBit Apr 3, 2020

reuvenlax Apr 3, 2020

TheNeuralBit Apr 3, 2020

reuvenlax Apr 3, 2020

TheNeuralBit commented Apr 3, 2020

TheNeuralBit left a comment

Use attachValues in SQL #11259

Use attachValues in SQL #11259

Conversation

reuvenlax commented Mar 29, 2020 • edited by alexvanboxel Loading

reuvenlax commented Mar 29, 2020

alexvanboxel commented Mar 30, 2020 • edited Loading

apilloud left a comment

Choose a reason for hiding this comment

apilloud commented Mar 30, 2020

reuvenlax commented Apr 1, 2020

apilloud commented Apr 1, 2020

reuvenlax commented Apr 2, 2020

apilloud commented Apr 2, 2020

TheNeuralBit commented Apr 3, 2020

TheNeuralBit left a comment

Choose a reason for hiding this comment

TheNeuralBit Apr 3, 2020

Choose a reason for hiding this comment

reuvenlax Apr 3, 2020

Choose a reason for hiding this comment

TheNeuralBit Apr 3, 2020

Choose a reason for hiding this comment

reuvenlax Apr 3, 2020

Choose a reason for hiding this comment

TheNeuralBit Apr 3, 2020

Choose a reason for hiding this comment

reuvenlax Apr 3, 2020

Choose a reason for hiding this comment

TheNeuralBit commented Apr 3, 2020

TheNeuralBit left a comment

Choose a reason for hiding this comment

reuvenlax commented Mar 29, 2020 •

edited by alexvanboxel

Loading

alexvanboxel commented Mar 30, 2020 •

edited

Loading