Skip to content

Conversation

@Chaho12
Copy link
Member

@Chaho12 Chaho12 commented Oct 21, 2024

Description

Fix parsing failure on WITH clauses.

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

* Fix parsing failures on WITH clauses. ({issue}`528`)

@Chaho12 Chaho12 self-assigned this Oct 21, 2024
@cla-bot cla-bot bot added the cla-signed label Oct 21, 2024
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

The changes look ok but overall I think we are doing the wrong thing here .. we really should not try to parse the query since we are missing the full context .. only the coordinator really has that. I think in the long run we have to go down the route of farming this work out to a coordinator .. otherwise we end up pulling everything in and duplicating it so we kinda end up with the whole parsing code again.

Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Please add a test.

@Chaho12
Copy link
Member Author

Chaho12 commented Oct 22, 2024

we really should not try to parse the query since we are missing the full context .. only the coordinator really has that.

I agree, even if it is quite helpful information. We should talk more on this next time.

@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/fix-temporary-table branch from 3e1cbb3 to 05d0600 Compare October 23, 2024 01:47
@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/fix-temporary-table branch from 05d0600 to cfca1ed Compare October 23, 2024 02:22
@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/fix-temporary-table branch from cfca1ed to f35c48c Compare October 23, 2024 02:57
@Chaho12 Chaho12 requested a review from ebyhr October 23, 2024 02:58
@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/fix-temporary-table branch from f35c48c to 998379d Compare October 23, 2024 05:05
Copy link
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

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

it looks like we are preserving with clause names in the tables list - I think we should exclude them. Lets discuss if needed

@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/fix-temporary-table branch from 2802539 to 32ddf59 Compare October 24, 2024 14:13
@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/fix-temporary-table branch from 32ddf59 to 40593ae Compare October 29, 2024 15:14
Comment on lines 348 to 355
case Table s -> {
// ignore temporary tables as they can have various table parts
if (!temporaryTables.contains(s.getName())) {
tableBuilder.add(qualifyName(s.getName()));
}
}
case TableFunctionInvocation s -> tableBuilder.add(qualifyName(s.getName()));
case WithQuery withQuery -> temporaryTables.add(QualifiedName.of(withQuery.getName().getValue()));
Copy link
Contributor

Choose a reason for hiding this comment

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

If you revert this to handle Query nodes, then we can bypass my concern about encountering a Table with a WithQuery name before it is added to temporaryTables. The concern is basically that node.getChildren() could return a list with the QueryBody before the With. If we explicitly process any With, then we can ensure that the names are collected before they are referenced. Something like

case Query query -> query.getWith().ifPresent(
                    with -> temporaryTables.addAll(with.getQueries().stream().map(WithQuery::getName).map(Identifier::getValue).map(QualifiedName::of).toList()));

Copy link
Member Author

Choose a reason for hiding this comment

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

bypass my concern about encountering a Table with a WithQuery name before it is added to temporaryTables

I don't think we should worry too much about this as they say that WITH clause must always come before select in various docs.

  • The WITH clause must always come at the beginning of the SQL statement, before any table is referenced in the main query. This placement allows the database engine to process the CTEs before executing the main query, ensuring that the defined temporary result sets are available for use throughout the rest of the statement

Oracle mysql docs also say that WITH clause must be placed at the beginning of the query or immediately preceding the SELECT statement in various contexts too.
image

But nonetheless it simplfies code so i changed it 👍

@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/fix-temporary-table branch 2 times, most recently from 9f55135 to 0033d36 Compare October 31, 2024 14:50
@mosabua
Copy link
Member

mosabua commented Oct 31, 2024

As discussed in the sync I think we should try to get this in asap and then cut 12 - I hope you can collab on that @willmostly @ebyhr and @Chaho12

Copy link
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

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

lgtm!

@willmostly willmostly merged commit a0de70f into trinodb:main Nov 1, 2024
2 checks passed
@github-actions github-actions bot added this to the 12 milestone Nov 1, 2024
@willmostly
Copy link
Contributor

Merging for release 12 - further fixups can be done as a follow up

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

Development

Successfully merging this pull request may close these issues.

Parsing fails when there isn't default schema

4 participants