[BEAM-11808][BEAM-9879] Support aggregate functions with two arguments #16200

benWize · 2021-12-10T21:27:01Z

Working on related issues

[BEAM-11808] Support aggregate functions with two arguments
[BEAM-9879] Support STRING_AGG in BeamSQL

Please add a meaningful description for your change here

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Twister2
Go	---	---
Java
Python	---	---
XLang		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

benWize · 2021-12-10T23:07:13Z

Hi R: @ibzib could you help me to review the current progress of this?
I made this change https://github.com/apache/beam/blob/d57f7c1ef3fff6b458cb8514460e157486d5648f/sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/SqlOperatorMappingTable.java#L116
to be able to get the delimiter from the argList here https://github.com/apache/beam/blob/d57f7c1ef3fff6b458cb8514460e157486d5648f/sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/SqlOperators.java#L183
because when this cast occurs, the argList is lost
https://github.com/apache/beam/blob/d57f7c1ef3fff6b458cb8514460e157486d5648f/sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/AggregateScanConverter.java#L235
Currently, the STRING_AGG's tests are passing https://ci-beam.apache.org/job/beam_PreCommit_SQL_Java11_Commit/3356/testReport/org.apache.beam.sdk.extensions.sql.zetasql/ZetaSqlDialectSpecTest/, ~~but some checkstyle jobs are failing~~ (Fixed)

benWize · 2021-12-28T16:50:17Z

R: @apilloud. Could you help me to review this?

...ava/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/udaf/StringAgg.java

...main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/AggregateScanConverter.java

...ain/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/SqlOperatorMappingTable.java

...tasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/SqlOperators.java

...ain/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/SqlOperatorMappingTable.java

ibzib · 2021-12-28T17:19:04Z

R: @apilloud. Could you help me to review this?

@benWize Sorry! I reviewed this, but I forgot to press send.

benWize · 2021-12-28T19:13:28Z

R: @apilloud. Could you help me to review this?

@benWize Sorry! I reviewed this, but I forgot to press send.

Thank you @ibzib, no worries.

ibzib · 2022-01-07T20:06:55Z

Apologies - I've been a bit busy this week but I'll give this another review next week.

ibzib

I ran this through ZetaSQL compliance tests (which we still haven't set up to run directly on the Beam repo) and there were a couple errors. Can you please try adding these tests to Beam and see what the issues are?

array_aggregation_test:array_agg_constant_value

SELECT ARRAY_AGG(1) b FROM UNNEST([1, 2, 3]) a

aggregation_queries_test:timestamp_NULL_min_max

SELECT MAX(CAST(NULL AS TIMESTAMP)) AS max_NULL,
       MIN(CAST(NULL AS TIMESTAMP)) AS min_NULL
FROM (SELECT 1)

...main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/AggregateScanConverter.java

ibzib · 2022-01-11T19:50:42Z

...main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/AggregateScanConverter.java

-          || expr.nodeKind() == RESOLVED_GET_STRUCT_FIELD) {
+      ZetaSQLResolvedNodeKind.ResolvedNodeKind resolvedNodeKind =
+          expr.getArgumentList().get(i).nodeKind();
+      if (i == 0 && resolvedNodeKinds.contains(resolvedNodeKind)) {


What about the case of a literal as the first argument? If I'm understanding the logic right, it doesn't throw an error even though it should (instead it continues).

I added both tests cases and also change the condition to throw an exception in the case of a literal as the first argument. Both tests fall in this case https://ci-beam.apache.org/job/beam_PreCommit_SQL_Commit/4616/.
The tests also failed before changing the condition but with a different exception

What was the other exception? Also, do you think it would be possible to support literals as the first argument? I'm not sure why that limitation exists.

Both are IndexOutOfBounds when inferring types https://ci-beam.apache.org/job/beam_PreCommit_SQL_Commit/4618/
I think it would be possible to support literals as the first argument, but we should verify every function/case.

Alright, for now let's do this:

Throw an UnsupportedOperationException when literals are passed as the first argument. In other words, keep the current behavior (prior to this PR).

Remove the new array_agg and timestamp test cases.

We can add support for literals later if we think it's important. I filed a separate JIRA: https://issues.apache.org/jira/browse/BEAM-13648

...main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/AggregateScanConverter.java

codecov · 2022-01-11T23:56:24Z

Codecov Report

Merging #16200 (3d92b4f) into master (0a220a1) will increase coverage by 8.98%.
The diff coverage is n/a.

❗ Current head 3d92b4f differs from pull request most recent head 1b69b20. Consider uploading reports for the commit 1b69b20 to get more accurate results

@@            Coverage Diff             @@
##           master   #16200      +/-   ##
==========================================
+ Coverage   74.60%   83.59%   +8.98%     
==========================================
  Files         653      452     -201     
  Lines       81798    62001   -19797     
==========================================
- Hits        61029    51830    -9199     
+ Misses      19788    10171    -9617     
+ Partials      981        0     -981

Impacted Files	Coverage Δ
sdks/python/apache_beam/utils/interactive_utils.py	`87.80% <0.00%> (-2.44%)`	⬇️
sdks/python/apache_beam/dataframe/transforms.py	`94.92% <0.00%> (-0.34%)`	⬇️
sdks/python/apache_beam/runners/common.py	`89.98% <0.00%> (-0.15%)`	⬇️
...eam/runners/portability/fn_api_runner/fn_runner.py	`90.80% <0.00%> (-0.11%)`	⬇️
sdks/python/apache_beam/transforms/util.py	`95.86% <0.00%> (-0.11%)`	⬇️
sdks/python/apache_beam/metrics/metric.py	`95.38% <0.00%> (ø)`
sdks/python/apache_beam/dataframe/expressions.py	`92.90% <0.00%> (ø)`
sdks/go/pkg/beam/core/graph/scope.go
sdks/go/pkg/beam/core/runtime/exec/hash.go
sdks/go/pkg/beam/transforms/stats/sum_switch.go
... and 201 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a220a1...1b69b20. Read the comment docs.

ibzib · 2022-01-12T19:55:39Z

I tried compliance testing from your second-most recent commit (5ea6102266e0f477e8cc4b9f0efb2831df72610b) and there's at least one more failing test. I think we should allow ResolvedParameter as the 2nd+ argument.

Name: aggregation_queries_test:aggregation_string_agg_error_1_null_string_param

  SELECT string_agg("s", @separator) FROM (SELECT 1)

Parameters:
@separator = String(NULL)

java.lang.ClassCastException: class com.google.zetasql.resolvedast.ResolvedNodes$ResolvedParameter cannot be cast to class com.google.zetasql.resolvedast.ResolvedNodes$ResolvedLiteral (com.google.zetasql.resolvedast.ResolvedNodes$ResolvedParameter and com.google.zetasql.resolvedast.ResolvedNodes$ResolvedLiteral are in unnamed module of loader 'app')
	at org.apache.beam.sdk.extensions.sql.zetasql.translation.SqlOperators.createStringAggOperator(SqlOperators.java:186)
	at org.apache.beam.sdk.extensions.sql.zetasql.translation.SqlOperatorMappingTable.create(SqlOperatorMappingTable.java:124)
	at org.apache.beam.sdk.extensions.sql.zetasql.translation.AggregateScanConverter.convertAggCall(AggregateScanConverter.java:238)
	at org.apache.beam.sdk.extensions.sql.zetasql.translation.AggregateScanConverter.convert(AggregateScanConverter.java:99)
	at org.apache.beam.sdk.extensions.sql.zetasql.translation.AggregateScanConverter.convert(AggregateScanConverter.java:55)
	at org.apache.beam.sdk.extensions.sql.zetasql.translation.QueryStatementConverter.convertNode(QueryStatementConverter.java:102)

benWize · 2022-01-14T01:16:34Z

I tried adding a new test with the query SELECT string_agg("s", @separator) FROM (SELECT 1) and It failed in

https://github.com/apache/beam/blob/f45b17242ef125e1ac0f303cc231e9bab614c207/sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/SqlOperators.java#L186

I tried adding a new case for ResolvedParameter but I couldn't get the value for delimiter there, it is undefined at that point, I'm not sure how I should handle that case.

ibzib · 2022-01-14T23:51:24Z

Let's leave parameters unsolved for now then, I think the compliance tests will be okay as long as we throw an UnsupportedOperationException instead of a ClassCastException. Would you mind also creating a jira so we can link it in the unsupported exception?

benWize · 2022-01-17T16:44:25Z

Let's leave parameters unsolved for now then, I think the compliance tests will be okay as long as we throw an UnsupportedOperationException instead of a ClassCastException. Would you mind also creating a jira so we can link it in the unsupported exception?

Ok, now is throwing UnsupportedOperationException and I filled https://issues.apache.org/jira/browse/BEAM-13673 to link it.

… with delimiter

ibzib

LGTM

FYI - I was wrong that UnsupportedOperationException was the right one to throw for parameters, so I had to change the exception to a different type to satisfy the compliance tests. But everything passes now.

benWize · 2022-01-18T21:49:04Z

Thank you @ibzib!

* github/master: (198 commits) Merge pull request apache#16369 from [BEAM-13558] [Playground] Hide the Graph tab and SCIO from SDK options Merge pull request apache#16546 from [BEAM-13661] [BEAM-13704] [Playground] Update tags for examples/katas/unit-tests Merge pull request apache#16540 from [BEAM-13678][Playground]Update Github Action To Deploy Examples [BEAM-13430] Re-add provided configuration (apache#16552) [BEAM-10206] Remove Fatalf calls in non-test goroutines for tests/benchmarks (apache#16575) [BEAM-13693] Bump beam_PostCommit_Java_ValidatesRunner_Dataflow_Streaming timeout to 12 hours (apache#16576) [BEAM-13699] Replace fnv with maphash. (apache#16573) Update Java FnAPI beam master (apache#16572) Merge pull request apache#16371 from [BEAM-13518][Playground] Beam Playground quickstart page on the Beam website Merge pull request apache#16373 from [BEAM-13515] [Playground] Hiding lines in an example that are not necessary Merge pull request apache#16472: [BEAM-13697] Add SchemaFieldNumber annotation [BEAM-13689] Output token elements when BQ batch writes complete. Disable logging for memoization test. (apache#16556) BEAM-13611 reactivating jdbcio xlang test Revert "Merge pull request apache#15863 from [BEAM-13184] Autosharding for JdbcIO.write* transforms" Remove obsolete commands from Inventory job. (apache#16564) [BEAM-13688] fixed type in BPG 4.5.3 window section (apache#16560) [BEAM-13683] Make cross-language SQL example pipeline (apache#16567) Merge pull request apache#16243 from darshan-sj/feature/support-priority-spannerio - Making rpcPriority a ValueProvider in SpannerConfig edited README and comments in Python multi-lang pipes examples Merge pull request apache#16518 from [BEAM-13619] [Playground] Add loading animation to the catalog Merge pull request apache#16519 from [BEAM-13639] [Playground] Add notification to Log/Output tabs about cached example Merge pull request apache#16533 from [BEAM-13548] [Playground] Add example description popover Merge pull request apache#16531 from [BEAM-13567] [playground] Handle run code validation and preparation errors Merge pull request apache#16370 from [BEAM-13556] playground - color and scroll tabs with new content [BEAM-13611] Skip test_xlang_jdbc_write (apache#16554) [BEAM-13015] Provide caching statistics in the status client. (apache#16495) Merge pull request apache#16309: [BEAM-13503] Set a default value to throwWriteErrors in BulkIO constructor [BEAM-13015] Add state caching capability to be used as hint for runners to not duplicate cached data if the SDK can do it for user state and side inputs. (apache#16525) [BEAM-13665] Make SpannerIO projectID optional again (apache#16547) Merge pull request apache#16322 from [BEAM-13407] [Playground] Preload fonts for the web application Merge pull request apache#16506 from [BEAM-13652][Playground] Send examples' links to the frontend [BEAM-11808][BEAM-9879] Support aggregate functions with two arguments (apache#16200) Update walkthrough.md (apache#16512) [BEAM-13683] Correct SQL transform schema, fix expansion address override bug (apache#16551) Merge pull request apache#16486 from [BEAM-13544][Playground] Add logs to examples CI/CD to see the progress [BEAM-13616][BEAM-13646] Upgrade vendored calcite to 1.28.0:0.2 (apache#16544) [BEAM-13616][BEAM-13645] Switch to vendored grpc 1.43.2 (apache#16543) Also bump FnAPI container. [BEAM-13680] Fixed code_repository (added pipelineUuid to RunCodeResult when status is "Finished") [BEAM-13616] Update com.google.cloud:libraries-bom to 24.2.0 (apache#16509) [BEAM-13430] Remove jcenter which will no longer contain any updates. (apache#16536) Update GH Actions to use proper variables names and proper triggers Remove jcenter repositories from gradle configuration. (apache#16532) Merge pull request apache#16507: [BEAM-13137] Fixes ES utest size flakiness with _flush api and index.store.stats_refresh_interval=0 [BEAM-13664] Fix Primitives hashing benchmark (apache#16523) Bump beam container version. [BEAM-12621] - Update Jenkins VMs to modern Ubuntu version (apache#16457) doc tweaks (apache#16498) Redirecting cross-language transforms content (apache#16504) Remove tab from source. Remove unnecessary fmt call in universal.go Clean up string cast of bytes in vet.go and corresponding tests fix capitalized error strings in expansionx Remove unnecessary blank identifier assignment in harness.go Replace string(buf.Bytes()) with buf.String() in coder_test.go Replace bytes.Compare() with bytes.Equal() in test cases Remove unnecessary fmt.Sprintf() in partition.go Fix staticcheck errors in transforms directory [BEAM-13590] Fix abc imports from collections (apache#15850) Merge pull request apache#16482 from [BEAM-13429][Playground] Add builder for preparers [BEAM-10206] Resolve go vet errors in protox package [BEAM-12572] Run java examples on multiple runners (apache#16450) [BEAM-13400] JDBC IO does not support UUID and JSONB PostgreSQL types and OTHER JDBC types in general [BEAM-13577] Beam Select's uniquifyNames function loses nullability of Complex types while inferring schema [BEAM-12164]: Add SDF for reading change stream records [BEAM-13455] Remove duplicated artifacts when using multiple environments with Dataflow Java Merge pull request apache#16485 from [BEAM-13486] [Playground] For unit tests (java) if one of tests fails the output goes to stdOutput Merge pull request apache#16385 from [BEAM-13535] [Playground] add cancel execution button [BEAM-12558] Fix doc typo. Merge pull request apache#16467 from [BEAM-12164]: SpannerIO DetectNewPartitions SDF Introduce the notion of a JoinIndex for fewer shuffles. (apache#16101) [BEAM-12464] Change ProtoSchemaTranslator beam schema creation to match the order for protobufs containing Oneof fields (apache#14974) Stronger typing inference for CoGBK. (apache#16465) [BEAM-13480] Increase pipeline timeout for PubSubIntegrationTest.test_streaming_data_only (apache#16496) Provide API to check whether a hint is known. [BEAM-8806] Integration test for SqsIO using Localstack Split builder into several builder for each step of pipeline execution [BEAM-13399] Move service liveness polling to Runner type (apache#16487) Merge pull request apache#16325 from [BEAM-13471] [Playground] Tag existing unit-tests Adds several example multi-language Python pipelines [BEAM-13616][BEAM-13646] Update vendored calcite 1.28.0 with protobuf 3.19.2 (apache#16473) Merge pull request apache#16374 from [BEAM-13398][Playground] Split LifeCycle to DTO and business logic Merge pull request apache#16363 from [BEAM-13557] [Playground] show code execution time Merge pull request apache#16149 from [BEAM-13113] [Playground] playground frontend documentation Merge pull request apache#16469 from [BEAM-13623][Playground] [Bugfix] During unit tests failing there is no any output [BEAM-13641][Playground] Add SCIO SDK support on the CI/CD step [BEAM-13638] Datatype of timestamp fields in SqsMessage for AWS IOs for SDK v2 was changed from String to long, visibility of all fields was fixed from package private to public [BEAM-13616] Initial files for vendored gRPC 1.43.2 (apache#16460) [BEAM-13432] Skip ExpansionService creation in Job Server (apache#16222) [BEAM-13628] Update SideInputCache to use full Transform and SideInputIDs as token information (apache#16483) [BEAM-13631] Add deterministic SQS message coder to fix reading from SQS in batch mode [BEAM-8806] Integration test for SqsIO [adhoc] Run spotlessApply on java examples to fix master Merge pull request apache#16156 from [BEAM-13391] Fix temporary file format in WriteToBigQuery Optional args and kwargs for named external transforms. [BEAM-13614] Add OnWindowExpiration support to the Java SDK harness and proto translation. (apache#16458) [BEAM-3221] Improve documentation in model pipeline protos (apache#16474) Merge pull request apache#16147 from [BEAM-13359] [Playground] Tag existing examples [BEAM-13626] Remap expanded outputs after merging. (apache#16471) ...

benWize changed the title ~~[WIP][BEAM-11808][BEAM-9879] Support aggregate functions with two arguments~~ [BEAM-11808][BEAM-9879] Support aggregate functions with two arguments Dec 15, 2021

benWize marked this pull request as ready for review December 15, 2021 21:17

ibzib reviewed Dec 28, 2021

View reviewed changes

benWize requested a review from ibzib January 3, 2022 16:39

ibzib reviewed Jan 11, 2022

View reviewed changes

benWize and others added 13 commits January 18, 2022 11:25

[BEAM-11808] Enable two params in aggregate functions, add string_agg…

eb0d35d

… with delimiter

[BEAM-11808] Fix checkstyle warning

b20ae71

[BEAM-11808] Fix spotlessApply

cd9032d

[BEAM-11808] Fix checkstyle warning

940977d

[BEAM-11808] Fix spotlessApply

f335217

[BEAM-11808] Change initialization StringAgg and minor fixes

5b44024

[BEAM-11808] Fix checkstyle warnings

5302e52

[BEAM-11808] Add test cases for array_agg and timestamp null max,min

cefe7fb

[BEAM-11808] Remove leftover file

f4411d4

[BEAM-11808] Enable resolved_literal as firts arg

618c69f

[BEAM-11808] Remove tests, validate RESOLVED_LITERAL as second argument

7ee7a5d

[BEAM-11808] Add unsupportedException for delimiter as ResolvedParam

16d9d4c

use zetasql exception

8760842

ibzib force-pushed the BEAM-11808 branch from a6c3e04 to 8760842 Compare January 18, 2022 20:24

update test

1b69b20

ibzib approved these changes Jan 18, 2022

View reviewed changes

ibzib merged commit 77e924f into apache:master Jan 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-11808][BEAM-9879] Support aggregate functions with two arguments #16200

[BEAM-11808][BEAM-9879] Support aggregate functions with two arguments #16200

benWize commented Dec 10, 2021

benWize commented Dec 10, 2021 •

edited

Loading

benWize commented Dec 28, 2021

ibzib commented Dec 28, 2021

benWize commented Dec 28, 2021

ibzib commented Jan 7, 2022

ibzib left a comment

ibzib Jan 11, 2022

benWize Jan 12, 2022

ibzib Jan 12, 2022

benWize Jan 12, 2022

ibzib Jan 12, 2022 •

edited

Loading

benWize Jan 14, 2022

codecov bot commented Jan 11, 2022 •

edited

Loading

ibzib commented Jan 12, 2022

benWize commented Jan 14, 2022

ibzib commented Jan 14, 2022

benWize commented Jan 17, 2022

ibzib left a comment

benWize commented Jan 18, 2022

[BEAM-11808][BEAM-9879] Support aggregate functions with two arguments #16200

[BEAM-11808][BEAM-9879] Support aggregate functions with two arguments #16200

Conversation

benWize commented Dec 10, 2021

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

benWize commented Dec 10, 2021 • edited Loading

benWize commented Dec 28, 2021

ibzib commented Dec 28, 2021

benWize commented Dec 28, 2021

ibzib commented Jan 7, 2022

ibzib left a comment

Choose a reason for hiding this comment

ibzib Jan 11, 2022

Choose a reason for hiding this comment

benWize Jan 12, 2022

Choose a reason for hiding this comment

ibzib Jan 12, 2022

Choose a reason for hiding this comment

benWize Jan 12, 2022

Choose a reason for hiding this comment

ibzib Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

benWize Jan 14, 2022

Choose a reason for hiding this comment

codecov bot commented Jan 11, 2022 • edited Loading

Codecov Report

ibzib commented Jan 12, 2022

benWize commented Jan 14, 2022

ibzib commented Jan 14, 2022

benWize commented Jan 17, 2022

ibzib left a comment

Choose a reason for hiding this comment

benWize commented Jan 18, 2022

`ValidatesRunner` compliance status (on master branch)

benWize commented Dec 10, 2021 •

edited

Loading

ibzib Jan 12, 2022 •

edited

Loading

codecov bot commented Jan 11, 2022 •

edited

Loading