Skip to content

[SPARK-35987][SQL] The ANSI flags of Sum and Avg should be kept after being copied#33186

Closed
gengliangwang wants to merge 4 commits intoapache:masterfrom
gengliangwang:sumAndAvg
Closed

[SPARK-35987][SQL] The ANSI flags of Sum and Avg should be kept after being copied#33186
gengliangwang wants to merge 4 commits intoapache:masterfrom
gengliangwang:sumAndAvg

Conversation

@gengliangwang
Copy link
Member

What changes were proposed in this pull request?

Make the ANSI flag part of expressions Sum and Average's parameter list, instead of fetching it from the sessional SQLConf.

Why are the changes needed?

For Views, it is important to show consistent results even the ANSI configuration is different in the running session. This is why many expressions like 'Add'/'Divide' making the ANSI flag part of its case class parameter list.

We should make it consistent for the expressions Sum and Average

Does this PR introduce any user-facing change?

Yes, the Sum and Average inside a View always behaves the same, independent of the ANSI model SQL configuration in the current session.

How was this patch tested?

Existing UT

@gengliangwang gengliangwang requested a review from cloud-fan July 2, 2021 09:04
@github-actions github-actions bot added the SQL label Jul 2, 2021
@SparkQA
Copy link

SparkQA commented Jul 2, 2021

Test build #140572 has finished for PR 33186 at commit 1e54428.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 2, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45083/

@SparkQA
Copy link

SparkQA commented Jul 2, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45083/

@SparkQA
Copy link

SparkQA commented Jul 4, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45130/

@SparkQA
Copy link

SparkQA commented Jul 4, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45130/

@SparkQA
Copy link

SparkQA commented Jul 4, 2021

Test build #140617 has finished for PR 33186 at commit c484538.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.


override protected def withNewChildInternal(newChild: Expression): Sum = copy(child = newChild)

// The flag `failOnError` won't be shown in the `toString` or `toAggString` methods
Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure, are these same with other expressions? other places might have to be fixed (in a separate PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

The basic math operations (Add, Subtract, etc.) just print something like a + b without showing the ANSI flag. This makes sense to me.

@cloud-fan
Copy link
Contributor

thanks, merging to master/3.2!

cloud-fan pushed a commit that referenced this pull request Jul 5, 2021
… being copied

### What changes were proposed in this pull request?

Make the ANSI flag part of expressions `Sum` and `Average`'s parameter list, instead of fetching it from the sessional SQLConf.

### Why are the changes needed?

For Views, it is important to show consistent results even the ANSI configuration is different in the running session. This is why many expressions like 'Add'/'Divide' making the ANSI flag part of its case class parameter list.

We should make it consistent for the expressions `Sum` and `Average`

### Does this PR introduce _any_ user-facing change?

Yes, the `Sum` and `Average` inside a View always behaves the same, independent of the ANSI model SQL configuration in the current session.

### How was this patch tested?

Existing UT

Closes #33186 from gengliangwang/sumAndAvg.

Authored-by: Gengliang Wang <gengliang@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
(cherry picked from commit 51103cd)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@cloud-fan cloud-fan closed this in 51103cd Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants