trino icon indicating copy to clipboard operation
trino copied to clipboard

Published 20 hours ago • verified publisher icon trinodb

Fix the issue of Function inside TABLESAMPLE

Open albericgenius opened this issue 2 years ago • 9 comments

Description

Is this change a fix, improvement, new feature, refactoring, or other?

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

  • I changed the StatementAnalyzer, if the resolvedFunctions have already existed, it will be reused. before the resolvedFunctions can not be find by following:
functionDecoder.fromQualifiedName(name)

How would you describe this change to a non-technical end user or system administrator?

  • This is a bug fix, no need to update any doc.

Related issues, pull requests, and links

https://github.com/trinodb/trino/issues/12107

Documentation

(+) No documentation is needed. ( ) Sufficient documentation is included in this PR. ( ) Documentation PR is available with #prnumber. ( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(+) No release notes entries required. ( ) Release notes entries required with the following suggested text:

# Section
* Fix he issue of Function inside TABLESAMPLE. ({issue}`12107`)

albericgenius avatar May 04 '22 10:05 albericgenius

  • This commit have not affect the TestDeltaTaskFailureRecoveryTest
  • I have rerun the test case (io.trino.plugin.deltalake.TestDeltaTaskFailureRecoveryTest) in my dev environment, these test cases can be successful as following.

cc @arhimondr @losipiuk

findepi avatar May 09 '22 07:05 findepi

The CI issue is a result of the test being flaky. The fix is already merged.

arhimondr avatar May 09 '22 19:05 arhimondr

The CI issue is a result of the test being flaky. The fix is already merged.

Thanks for your information, I have merged the latest code into my branch, All the CI test cases are passed

albericgenius avatar May 10 '22 07:05 albericgenius

@findepi，@dain， @raunaqmorarka and @losipiuk

  • Welcome for any comments, I will update as soon as possible.
  • All the CI test cases are passed

albericgenius avatar May 11 '22 11:05 albericgenius

@findepi，@dain， @raunaqmorarka and @losipiuk I have do some cleanups for my commits:

  • I merged two commits to the single one.
  • I have rebased my changes.

Thanks for your time and help.

albericgenius avatar May 18 '22 12:05 albericgenius

Hey I wonder can we have this fix merge?

puchengy avatar May 31 '22 23:05 puchengy

The underlying issue is that we're attempting to evaluate an expression from the syntax tree with incomplete analysis. A proper fix would be to:

  • change StatementAnalyzer.visitTableSample to just analyze the expression and validate the type of the expression and whether it's constant (technically, not required by the SQL spec, but it's a limitation we have since we don't currently support decorrelating column references in SampleNode)
  • update the planner to translate the expression into an IR expression
  • add support for simplifying (constant folding) the expression in SampleNode during query optimization
  • add a runtime check to ensure the sample percentage is in the proper range

The fix proposed in this PR just patches around the problem, and doesn't solve other issues like, for instance, lack of support for constant expressions that contain lambda expressions.

@martint @findepi Thanks for your help and time I found the IrExpression have not merged into master branch yet(May be I am wrong). I do not know how IR expression work.

  • If we already have IR expression in master branch, could you please help to provide a little information about IR expression.
  • if not, after IrExpression merged, I will learn how it work and fix this later.

Thanks

albericgenius avatar Sep 20 '22 10:09 albericgenius

@albericgenius IrExpression is an implementation of the "IR Expression" concept, but fixing TABLESAMPLE+functions should not require https://github.com/trinodb/trino/pull/13690

I don't know how @martint 's comment https://github.com/trinodb/trino/pull/12237#pullrequestreview-1006480783 relates to the PR code, since the PR currently appears empty. Therefore cannot try to come up with a better explanation that the above.

BTW it's recommended to make PRs from some other branch than master. Github has nice writeups on this, see e.g. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-and-deleting-branches-within-your-repository

findepi avatar Sep 20 '22 10:09 findepi

@findepi I rebased the code with existed code and reopen it. Welcome for any comment, I will update as soon as possible.

albericgenius avatar Sep 20 '22 13:09 albericgenius