Skip to content

Ensure Parameters are in proper order for queries having WITH clause#1529

Merged
martint merged 2 commits intotrinodb:masterfrom
Praveen2112:parameter_changes
Sep 24, 2019
Merged

Ensure Parameters are in proper order for queries having WITH clause#1529
martint merged 2 commits intotrinodb:masterfrom
Praveen2112:parameter_changes

Conversation

@Praveen2112
Copy link
Copy Markdown
Member

Fixes #1191

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will produce an ordering based on traversal order. We need to:

  1. Collect all the Parameter nodes
  2. Sort them based on their lexical order (Parameter.getLocation())

Copy link
Copy Markdown
Member

@martint martint left a comment

Choose a reason for hiding this comment

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

Instead of passing parameters and parameterPositionMap everywhere, why not compose them into a single mapping of parameter -> expression (i.e., Map<NodeRef<Parameter>, Expression>) in Analysis and pass that around?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of assembling these into a long, compare based on line first and then based on column number.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's already a class that does this: ParameterExtractor.

This could all be replaced with:

List<Parameter> parameters = getParameters(...).stream()
        .sorted(Comparator.comparing(
                parameter -> parameter.getLocation().get(),
                Comparator.comparing(NodeLocation::getLineNumber)
                        .thenComparing(NodeLocation::getColumnNumber)))
        .collect(Collectors.toList());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The type of this should be Map<NodeRef<Parameter>, Integer>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Make this a Map<NodeRef<Parameter>, Integer> and rename the variable to parameterOrdinals

@martint
Copy link
Copy Markdown
Member

martint commented Sep 17, 2019

Can you add an end-to-end test that involves WITH to make sure parameters are being interpreted correctly?

@Praveen2112 Praveen2112 force-pushed the parameter_changes branch 2 times, most recently from 92f1af5 to 79f9e7d Compare September 18, 2019 12:23
@martint martint merged commit 51fdb53 into trinodb:master Sep 24, 2019
@Praveen2112 Praveen2112 deleted the parameter_changes branch November 9, 2019 14:16
v-jizhang added a commit to v-jizhang/presto that referenced this pull request Feb 24, 2022
Cherry-pick of trinodb/trino#1529

Fixes prestodb#17012

Co-authored-by: praveenkrishna <praveenkrishna@tutanota.com>
zhenxiao pushed a commit to prestodb/presto that referenced this pull request Mar 1, 2022
Cherry-pick of trinodb/trino#1529

Fixes #17012

Co-authored-by: praveenkrishna <praveenkrishna@tutanota.com>
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.

Wrong parameters order in EXECUTE USING when using with clause

2 participants