[FLINK-39293][table-planner] Fix MATCH_RECOGNIZE uses through view

[ldadima]

What is the purpose of the change

Fix for FLINK-39293, partially revert for FLINK-38493

Verifying this change

Run MatchRecognizeITCase#testSimplePatternInProcTimeWithTemporaryView

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]

CI report:

• fe3df81 Azure: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot run azure re-run the last Azure build

[snuyanzin]

ideally we should keep it as is as it gives fully expanded sql and expandSqlIdentifiers doesn't do that (for instance it doesn't expand star).

I tend to think that rather than fixing here it should be fixed in Calcite
for more details look at https://issues.apache.org/jira/browse/CALCITE-7465, https://issues.apache.org/jira/browse/CALCITE-7466, https://issues.apache.org/jira/browse/CALCITE-7467, probably something else

[snuyanzin]

also https://issues.apache.org/jira/browse/CALCITE-7470

[ldadima]

The problem, as I see it, is somewhat similar to the LATERAL issue: expandSqlIdentifiers was being used until the Calcite version (problem resolved) was updated.

Is correct usage of MATCH_RECOGNIZE in CREATE VIEW syntax important to us? If so, we can keep using expandSqlIdentifiers until upgrading Calcite version (1.42.0).
What do you think?

[snuyanzin]

the LATERAL issue: expandSqlIdentifiers was being used until the Calcite version (problem resolved) was updated

this is not entirely true
LATERAL was fixed in https://issues.apache.org/jira/browse/CALCITE-7217 and https://issues.apache.org/jira/browse/CALCITE-7312 which are 1.41.0 and 1.42.0 (not even released yet) and Flink master depends on 1.36.0

Calcite upgrade task is not an easy one because of lots of customizations
So we just port fixes from Calcite and remove such fixes when upgrade to certain version
for that reason you can find calcite classes in Flink repo e.g. here https://github.com/apache/flink/tree/master/flink-table/flink-table-planner/src/main/java/org/apache/calcite

[snuyanzin]

I would follow similar approach for MATCH_RECOGNIZE however first need to be sure that all cases are fixed in Calcite, for now I'm aware of at least 3-5 more cases which have to be fixed
for instance https://issues.apache.org/jira/browse/CALCITE-7471

[ldadima]

Yes, indeed, you are right.
But then I don't understand why solution for FLINK-38493 is correct. Can you help me to understand?

Code before FLINK-38493

[snuyanzin]

it was a workaround because of a bug with LATERAL in Calcite and there was a huge comment trying to explain why we need it.
The problem became more serious with MATERIALIZED TABLEs where we might want to use expanded query.
Also if you try to look at other vendors: they also expand star which Flink didn't do.

With this issue we see that we are in a lack of tests for MATCH_RECOGNIZE (looks like Calcite as well) and that's why it was not detected earlier.

for short term we could consider rolling it back, however so far nobody was heavily complaining that hot fix is urgently required. In this case I think we still have time to fix it in a proper way

[ldadima]

Thanks for explaining
Now it's clear

[nictownsend]

Hi both - thanks for the discussion and clarification.

for short term we could consider rolling it back, however so far nobody was heavily complaining that hot fix is urgently required. In this case I think we still have time to fix it in a proper way

I tend to build my SQL as a series of views, making it more composable and easier to read. E.g (pseudo):

CREATE TABLE source_1:
CREATE TABLE source_2;
CREATE TEMPORARY VIEW joined as (source_1 JOIN source_2);
CREATE TEMPORARY_VIEW matches as (joined MATCH_RECOGNIZE);
CREATE TABLE sink_1;
CREATE TABLE sink_1;
INSERT into sink_1 select * from matches;
INSERT into sink_2 select * from matches WHERE x=y;

Are there other ways to achieve the same effect without this bug being fixed? My concern is that I now have to write (pseduo):

CREATE TABLE source_1:
CREATE TABLE source_2;
CREATE TABLE sink_1;
CREATE TABLE sink_2;
INSERT into sink_1 select * from (source_1 JOIN source_1) MATCH_RECOGNIZE;
INSERT into sink_2 select * from ((source_1 JOIN source_1) MATCH_RECOGNIZE) WHERE x=y;

[snuyanzin]

for. now it is impossible if you use 2.2.0
the fixes in Calcite are still in progress however not ported to Flink repo yet (since not all cases are fixed)

I hope to have it done this/next week, however it means that the fixes might land to Flink branches only. Releases should be done separately

[raminqaf]

Are these changes related to the issue yo are trying to solve?

[ldadima]

Essentially, this pull request is a partial revert of FLINK-38493, so I also had to revert the test changes.
But, as you can see above in PR's discussion, was decided to close PR

[raminqaf]

Should we consider adding a second test variant with explicit columns alongside this one?