[hotfix][docs]fix flink sql Cascading Window TVF Aggregation exception #18414

simenliuxing · 2022-01-20T07:07:20Z

What is the purpose of the change

Aliases need to be used under Cascading Window Aggregation, otherwise an error will be reported, but there is no such thing in the documentation, so I fixed it

Brief change log

Fix errors in documentation, add aliases

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (no)

flinkbot · 2022-01-20T07:11:16Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 3c3aede (Thu Jan 20 07:11:16 UTC 2022)

✅no warnings

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2022-01-20T07:13:28Z

CI report:

2d7b1b7 Azure: SUCCESS
e59d3cd Azure: PENDING

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

simenliuxing · 2022-01-21T01:46:36Z

@MartijnVisser Can you review it for me?

beyond1920

@simenliuxing Thanks for contribution. LGTM.
Only left a few comments here.

beyond1920 · 2022-01-21T02:23:53Z

docs/content.zh/docs/dev/table/sql/queries/window-agg.md

@@ -199,7 +199,7 @@ The following shows a cascading window aggregation where the first window aggreg
 ```sql
 -- tumbling 5 minutes for each supplier_id
 CREATE VIEW window1 AS
-SELECT window_start, window_end, window_time as rowtime, SUM(price) as partial_price
+SELECT window_start as window1_start, window_end as window1_end, window_time as rowtime, SUM(price) as partial_price


Could you add some comments here to explain that we need alias here to avoid name conflict?

@beyond1920

When i use sql as below:

-- tumbling 5 minutes for each supplier_id CREATE VIEW window1 AS SELECT window_start as window1_start, window_end as window1_end, window_time as rowtime, SUM(price) as partial_price FROM TABLE( TUMBLE(TABLE Bid, DESCRIPTOR(bidtime), INTERVAL '5' MINUTES)) GROUP BY supplier_id, window_start, window_end, window_time; -- tumbling 10 minutes on the first window SELECT window_start, window_end, SUM(partial_price) as total_price FROM TABLE( TUMBLE(TABLE window1, DESCRIPTOR(rowtime), INTERVAL '10' MINUTES)) GROUP BY window_start, window_end;

The following exception occurs:

Caused by: org.apache.calcite.sql.validate.SqlValidatorException: Column 'window_start' is ambiguous at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.lang.reflect.Constructor.newInstance(Constructor.java:423) at org.apache.calcite.runtime.Resources$ExInstWithCause.ex(Resources.java:467) at org.apache.calcite.runtime.Resources$ExInst.ex(Resources.java:560) ... 41 more

I think the following piece of sql cannot identify whether window_start is from window1 or from built-in.
But the following sql is for Group Window Aggregation, the required window_start and window_end are not from window1, So I renamed window_start and window_end in the above sql.

I'm not sure if that a proper fix is changing the documentation. I would expect that we should address the issue to avoid getting the exception.

ok i see. this is the result of other communication with me
https://issues.apache.org/jira/browse/FLINK-25700?filter=-2

I see. If that's the case, then I guess we should indeed update this documentation :) I would then only slightly modify the alias to make it more clearer, something like:

Suggested change

SELECT window_start as window1_start, window_end as window1_end, window_time as rowtime, SUM(price) as partial_price

SELECT window_start as window_5mintumble_start, window_end as window_5mintumble_end, window_time as rowtime, SUM(price) as partial_price

or

Suggested change

SELECT window_start as window1_start, window_end as window1_end, window_time as rowtime, SUM(price) as partial_price

SELECT window_start as window_5mintumblepersupplier_start, window_end as window_5mintumblepersupplier_end, window_time as rowtime, SUM(price) as partial_price

Thanks for your suggestion, I have revised

Looks good to me, what do you think @beyond1920 ?

@simenliuxing Thanks for explain.
I mean to add some comments in documents to explain that we need alias here to avoid name conflict.

@beyond1920
Sorry for misunderstanding, I have added the comment

…d name conflict

beyond1920 · 2022-01-24T02:41:48Z

docs/content.zh/docs/dev/table/sql/queries/window-agg.md

@@ -199,7 +199,8 @@ The following shows a cascading window aggregation where the first window aggreg
 ```sql
 -- tumbling 5 minutes for each supplier_id
 CREATE VIEW window1 AS
-SELECT window_start, window_end, window_time as rowtime, SUM(price) as partial_price
+-- under the Cascading Window Aggregation to avoid field ambiguity window_start and window_end need to be renamed


Suggested change

-- under the Cascading Window Aggregation to avoid field ambiguity window_start and window_end need to be renamed

-- Note: The window start and window end fields of inner Window TVF are optional in the select clause. However, if they appear in the clause, they need to be aliased to prevent name conflicting with the window start and window end of the outer Window TVF.

WDTY?

Looks good to me, I have revised

…void name conflict

beyond1920 · 2022-01-24T08:27:37Z

Merged. Thanks for contribution @simenliuxing

apache#18414)

[hotfix][docs]fix flink sql Cascading Window TVF Aggregation exception

3c3aede

beyond1920 reviewed Jan 21, 2022

View reviewed changes

simenliuxing added 2 commits January 21, 2022 14:00

[hotfix][docs]modify the alias to make it more clearer

e44c4ea

[hotfix][docs]add comments to explain that we need alias here to avoi…

2d7b1b7

…d name conflict

simenliuxing requested a review from beyond1920 January 24, 2022 01:40

beyond1920 reviewed Jan 24, 2022

View reviewed changes

[hotfix][docs]modify comments to explain that we need alias here to a…

e59d3cd

…void name conflict

beyond1920 merged commit 2a3ff01 into apache:master Jan 24, 2022

niklassemmler pushed a commit to niklassemmler/flink that referenced this pull request Feb 3, 2022

[hotfix][docs]fix flink sql Cascading Window TVF Aggregation exception (

3b8e378

apache#18414)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[hotfix][docs]fix flink sql Cascading Window TVF Aggregation exception #18414

[hotfix][docs]fix flink sql Cascading Window TVF Aggregation exception #18414

simenliuxing commented Jan 20, 2022

flinkbot commented Jan 20, 2022

flinkbot commented Jan 20, 2022 •

edited

Loading

simenliuxing commented Jan 21, 2022

beyond1920 left a comment

beyond1920 Jan 21, 2022

simenliuxing Jan 21, 2022 •

edited

Loading

MartijnVisser Jan 21, 2022

simenliuxing Jan 21, 2022

MartijnVisser Jan 21, 2022

simenliuxing Jan 21, 2022

MartijnVisser Jan 21, 2022

beyond1920 Jan 21, 2022

simenliuxing Jan 21, 2022

beyond1920 Jan 24, 2022

simenliuxing Jan 24, 2022

beyond1920 commented Jan 24, 2022

	SELECT window_start as window1_start, window_end as window1_end, window_time as rowtime, SUM(price) as partial_price
	SELECT window_start as window_5mintumble_start, window_end as window_5mintumble_end, window_time as rowtime, SUM(price) as partial_price

	-- under the Cascading Window Aggregation to avoid field ambiguity window_start and window_end need to be renamed
	-- Note: The window start and window end fields of inner Window TVF are optional in the select clause. However, if they appear in the clause, they need to be aliased to prevent name conflicting with the window start and window end of the outer Window TVF.

[hotfix][docs]fix flink sql Cascading Window TVF Aggregation exception #18414

[hotfix][docs]fix flink sql Cascading Window TVF Aggregation exception #18414

Conversation

simenliuxing commented Jan 20, 2022

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Jan 20, 2022

Automated Checks

Review Progress

flinkbot commented Jan 20, 2022 • edited Loading

CI report:

simenliuxing commented Jan 21, 2022

beyond1920 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simenliuxing Jan 21, 2022 • 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

beyond1920 commented Jan 24, 2022

flinkbot commented Jan 20, 2022 •

edited

Loading

simenliuxing Jan 21, 2022 •

edited

Loading