Skip to content

[GLUTEN-3962][VL] Respect parsed attribute name and remove column name validate logic - #3963

Merged
waitinfuture merged 1 commit into
apache:mainfrom
Yohahaha:attr-name
Dec 13, 2023
Merged

[GLUTEN-3962][VL] Respect parsed attribute name and remove column name validate logic#3963
waitinfuture merged 1 commit into
apache:mainfrom
Yohahaha:attr-name

Conversation

@Yohahaha

@YohahahaYohahaha commented Dec 7, 2023

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Respect parsed attribute name from Spark, sum(a) is a valid attr name in Spark.

Remove column name validate logic safely since SparkTokenizer already added.

(Fixes: #3962)

How was this patch tested?

added UT.

@github-actions

github-actionsBot commented Dec 7, 2023

Copy link
Copy Markdown

#3962

@github-actions

github-actionsBot commented Dec 7, 2023

Copy link
Copy Markdown

Run Gluten Clickhouse CI

@Yohahaha

Yohahaha commented Dec 7, 2023

Copy link
Copy Markdown
ContributorAuthor

Spark33 allow read parquet column with special characters but Spark32 does not.... details see apache/spark#35229

@github-actions

github-actionsBot commented Dec 7, 2023

Copy link
Copy Markdown

Run Gluten Clickhouse CI

@github-actions

github-actionsBot commented Dec 7, 2023

Copy link
Copy Markdown

Run Gluten Clickhouse CI

@Yohahaha

Yohahaha commented Dec 8, 2023

Copy link
Copy Markdown
ContributorAuthor

@philo-he@rui-mo@ulysses-you please help review, thank you!

philo-he
philo-he reviewed Dec 8, 2023
View reviewed changes

@philo-hephilo-he left a comment

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.

Thanks for your work! Just few comments.

Comment threadbackends-velox/src/test/scala/io/glutenproject/execution/TestOperator.scala Outdated
Comment threadcpp/velox/substrait/SubstraitToVeloxPlanValidator.cc Outdated
@github-actions

github-actionsBot commented Dec 8, 2023

Copy link
Copy Markdown

Run Gluten Clickhouse CI

1 similar comment
@github-actions

github-actionsBot commented Dec 8, 2023

Copy link
Copy Markdown

Run Gluten Clickhouse CI

ulysses-you
ulysses-you reviewed Dec 8, 2023
View reviewed changes
Comment threadbackends-velox/src/test/scala/io/glutenproject/execution/TestOperator.scala Outdated
Comment threadgluten-core/src/main/scala/io/glutenproject/expression/ConverterUtils.scala Outdated
@github-actions

github-actionsBot commented Dec 10, 2023

Copy link
Copy Markdown

Run Gluten Clickhouse CI

rui-mo
rui-mo reviewed Dec 11, 2023
View reviewed changes
Comment threadgluten-core/src/main/scala/io/glutenproject/expression/ConverterUtils.scala Outdated
@github-actions

github-actionsBot commented Dec 11, 2023

Copy link
Copy Markdown

Run Gluten Clickhouse CI

@Yohahaha

Yohahaha commented Dec 11, 2023

Copy link
Copy Markdown
ContributorAuthor

cc @waitinfuture please help rerun failed ci job, thank you!

@github-actions

github-actionsBot commented Dec 11, 2023

Copy link
Copy Markdown

Run Gluten Clickhouse CI

@Yohahaha
fix
82a8566
fix
fix format
fix ut
fix
@github-actions

github-actionsBot commented Dec 12, 2023

Copy link
Copy Markdown

Run Gluten Clickhouse CI

@Yohahaha

Yohahaha commented Dec 12, 2023

Copy link
Copy Markdown
ContributorAuthor

@philo-he@ulysses-you@rui-mo any more comments?

philo-he
philo-he approved these changes Dec 12, 2023
View reviewed changes

@philo-hephilo-he left a comment

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.

Looks good to me! Thanks!

waitinfuture
waitinfuture approved these changes Dec 13, 2023
View reviewed changes

@waitinfuturewaitinfuture left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks!

@waitinfuture
waitinfuture merged commit 8edcb72 into apache:mainDec 13, 2023
@Yohahaha
Yohahaha deleted the attr-name branch December 13, 2023 05:45
@GlutenPerfBot

GlutenPerfBot commented Dec 13, 2023

Copy link
Copy Markdown
Contributor

===== Performance report for TPCH SF2000 with Velox backend, for reference only ====

querylog/native_3963_time.csvlog/native_master_12_12_2023_8c6f1643e_time.csvdifferencepercentage
q133.8933.75-0.14099.59%
q227.4824.88-2.59590.56%
q337.5837.710.124100.33%
q439.5137.86-1.65695.81%
q572.2372.830.599100.83%
q67.027.050.029100.41%
q783.9686.502.538103.02%
q887.9786.24-1.72698.04%
q9126.84125.50-1.33998.94%
q1045.7144.68-1.02697.76%
q1120.9420.21-0.72696.53%
q1227.3925.06-2.32691.51%
q1346.8146.58-0.23499.50%
q1415.6119.994.382128.08%
q1529.1127.55-1.56694.62%
q1615.4915.860.367102.37%
q17103.03103.270.238100.23%
q18150.83151.330.502100.33%
q1912.8313.891.060108.26%
q2026.9628.681.729106.41%
q21226.18229.122.941101.30%
q2214.0513.95-0.09399.34%
total1251.411252.491.083100.09%

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

Reviewers

@zzcclpzzcclpzzcclp left review comments
@ulysses-youulysses-youulysses-you left review comments
@rui-morui-morui-mo left review comments
@waitinfuturewaitinfuturewaitinfuture approved these changes
@philo-hephilo-hephilo-he approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CORE] Do not modify attribute name in transform scan operator

7 participants

@Yohahaha@GlutenPerfBot@waitinfuture@zzcclp@ulysses-you@philo-he@rui-mo