-
Notifications
You must be signed in to change notification settings - Fork 453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[UT] Add a tool to validate any unary expression with all its accepted types #6392
Conversation
Thanks for opening a pull request! Could you open an issue for this pull request on Github Issues? https://github.com/apache/incubator-gluten/issues Then could you also rename commit message and pull request title in the following format?
See also: |
Run Gluten Clickhouse CI |
247b6c6
to
f1f4ab6
Compare
Run Gluten Clickhouse CI |
@rui-mo, do you have any comment? |
Does the change apply to CH backend as well? |
CH backend will also run this. But as it doesn't implement doValidate API for native validation, this tool doesn't produce correct result. I will disable it for CH backend. Thanks! |
// scalastyle:off | ||
logInfo("!! cast validation fails: cast from " + from + " to " + to) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should every // scalastyle:off
requires for a corresponding // scalastyle:on
? I am not sure about it but didn't see independent usages of // scalastyle:off
before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrongly kept. I will remove it. Thanks!
Run Gluten Clickhouse CI |
When doing expression converter, it can go into CH's expression validation path in scala code. So maybe we can keep it run for CH. cc @zzcclp |
@zhztheplayer, do you have any other comment? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
val castExpr = Cast(generateChildExpression(from), to) | ||
if (castExpr.checkInputDataTypes().isSuccess) { | ||
val glutenProject = generateGlutenProjectPlan(castExpr) | ||
if (glutenProject.doValidate().isValid) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the api of the ValidationResult
had already changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zzcclp, yes, I will fix it soon. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI. Just noted hongze's fix: #6418
…ll its accepted types (apache#6392)" This reverts commit 86e5f9d.
…ll its accepted types (apache#6392)" This reverts commit 86e5f9d.
…ll its accepted types (apache#6392)" This reverts commit 86e5f9d.
…ll its accepted types (apache#6392)" This reverts commit 86e5f9d.
What changes were proposed in this pull request?
There are a few functions that are only partially supported, e.g., some input types are not supported. This PR is just for helping us to know the gaps.
This PR gets expression list and registered expression builders from Spark. In the validation process, the expression's input data doesn't really matter, but input type does. So we can use literal NULL child with specific type to build an expression instance. After this, we simply construct a Gluten plan with a dummy child node. Then, go into GlutenPlan's validation code path.
This is just the first patch for project function validation. Will have more PRs for other functions, like binary expressions, aggregate functions, etc.
See partial output: