Skip to content

[FLINK-38493][table] Port Calcite's fix for LATERAL is lost while validation #27096

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[FLINK-38493][table] Port Calcite's fix for LATERAL is lost while validation #27096

wants to merge 1 commit into from

Conversation

snuyanzin
Copy link
Contributor

@snuyanzin snuyanzin commented Oct 9, 2025 •
edited
Loading

What is the purpose of the change

The problem is CALCITE-7217 where LATERAL is lost while validation

the issue appeared after CALCITE-4077 (Calcite 1.24) and there are several places in Flink code with WAs
The PR is to have it fixed

Brief change log

Calcite classes and tests

Verifying this change

View and MaterializedTable related tests
especially test with LATERAL for materialized tables

flink/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlMaterializedTableNodeToOperationConverterTest.java

Line 158 in 47478ed

void testCreateMaterializedTableWithUDTFQuery() {

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? (not applicable )

@flinkbot
Copy link
Collaborator

flinkbot commented Oct 9, 2025 •
edited
Loading

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@snuyanzin snuyanzin force-pushed the lateral branch 2 times, most recently from 9a2c0ca to 78d281f Compare October 9, 2025 12:25
snuyanzin
snuyanzin commented Oct 9, 2025
View reviewed changes
flink-table/flink-sql-client/src/test/resources/sql/view.q
)
AS SELECT *
FROM `default_catalog`.`default_database`.`orders`
AS SELECT `orders`.`user`, `orders`.`product`, `orders`.`amount`, `orders`.`ts`, `orders`.`ptime`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since now validated query is used, star is expanded

@davidradl
Copy link
Contributor

@snuyanzin I think the PR title should be LATERAL not LITERAL

@snuyanzin snuyanzin changed the title [FLINK-38493][table] Port Calcite's fix for LITERAL is lost while validation [FLINK-38493][table] Port Calcite's fix for LATERAL is lost while validation Oct 9, 2025
@snuyanzin
Copy link
Contributor Author

yep, you are right, thank you, changed PR description
will change commit message later while merging

twalthr
twalthr reviewed Oct 9, 2025
View reviewed changes
.../src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConvertUtils.java
// the issue was resolved in CALCITE-4077
// (always treat the table function as implicitly LATERAL).
String expandedQuery = context.expandSqlIdentifiers(originalQuery);
String expandedQuery = context.toQuotedSqlString(query);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know how long this has been in place? It seems like a fundamental change to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems more than 5 years...
we can go in a bit lighter way: apply this only for materialized tables and leave old behavior for views

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but it was in place for some years before 2020? So LATERAL was the only problem. I like the expansion behavior, I just want to make sure that we are not breaking views.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twalthr twalthr twalthr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@snuyanzin @flinkbot @davidradl @twalthr