Skip to content

Fix incorrect order for parameters in DESCRIBE INPUT#14914

Merged
martint merged 5 commits intotrinodb:masterfrom
martint:describe-params
Nov 8, 2022
Merged

Fix incorrect order for parameters in DESCRIBE INPUT#14914
martint merged 5 commits intotrinodb:masterfrom
martint:describe-params

Conversation

@martint
Copy link
Copy Markdown
Member

@martint martint commented Nov 4, 2022

The parameters were being listed in depth-first-search order instead of in the order in which they appear in the query text.

Fixes #14738

Release notes

(x) Release notes are required, please propose a release note for me.

# General
* Fix incorrect order of parameters in `DESCRIBE INPUT` when they appear in a `WITH` clause. ({issue}`14738 `)

@cla-bot cla-bot bot added the cla-signed label Nov 4, 2022
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.

Can we be a bit more specific here like bindParametersToExpression the method name might mislead us ParameterRewriter - where we bind the parameter to expression

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think the "toExpression" suffix adds much value. Although, it'd be clearer if the second argument were named "values" instead of parameters -- the parameters are in the query itself.

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.

But if it is some sort of a uniqueId (which I totally agree) then comparing getId but parameters size would make less sense.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's sort of true, unless we define the ids as being from 0 to the number of parameters in the query. I hesitated removing these checks, but I do agree they look wonky. Let me see if there's a better way to represent them, or I'll just remove them.

The position does not actually match the order in
which they are supposed to be processed. It's
needed just to assign a unique identity to each parameter node.
They were being listed in depth-first-search order instead
of in the order in which they appear in the query text.
@martint martint merged commit 6c5ac2e into trinodb:master Nov 8, 2022
@martint martint deleted the describe-params branch November 8, 2022 19:04
@github-actions github-actions bot added this to the 403 milestone Nov 8, 2022
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.

DESCRIBE INPUT incorrectly orders binding variables on queries with CTEs

2 participants