Skip to content

Conversation

@vitaliili-db
Copy link
Contributor

What changes were proposed in this pull request?

This PR removes extra curly bracket from debug string for Decimal type in SQL.

This is a backport from master branch. Commit: 165ce4e

Why are the changes needed?

Typo in error messages of decimal overflow.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By running tests:

$ build/sbt "sql/testOnly"

This PR removes extra curly bracket from debug string for Decimal type in SQL.

Typo in error messages of decimal overflow.

No.

By running updated test:
```
$ build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z decimalArithmeticOperations.sql"
```

Closes apache#36397 from vli-databricks/SPARK-39060.

Authored-by: Vitalii Li <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
@github-actions github-actions bot added the SQL label May 5, 2022
@vitaliili-db
Copy link
Contributor Author

@MaxGekk please take a look

Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for CI.

@MaxGekk MaxGekk changed the title [SPARK-39060][SQL] Typo in error messages of decimal overflow [SPARK-39060][SQL][3.2] Typo in error messages of decimal overflow May 5, 2022
@vitaliili-db
Copy link
Contributor Author

@MaxGekk There are two tests failing in TPCDSV1_4_PlanStabilitySuite, but I also see that in the test run for the most recent commit to branch-3.2: #36341

@vitaliili-db
Copy link
Contributor Author

@MaxGekk Also I noticed that interval.sql.out is modified by #31349 but the change is not in master or any other branch.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@MaxGekk
Copy link
Member

MaxGekk commented May 6, 2022

There are two tests failing in TPCDSV1_4_PlanStabilitySuite, but I also see that in the test run for the most recent commit to branch-3.2: #36341

Maybe the tests are flaky. Could you re-trigger the GAs via empty commit:

$ git commit --allow-empty -m "Trigger build"

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

According to the log, TPCDSV1_4_PlanStabilitySuite failed with two test cases.

2022-05-06T21:20:15.6293676Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m*** 2 TESTS FAILED ***�[0m�[0m
2022-05-06T21:20:15.6573289Z �[0m[�[0m�[31merror�[0m] �[0m�[0mFailed: Total 9595, Failed 2, Errors 0, Passed 9593, Ignored 29�[0m
2022-05-06T21:20:15.6790224Z �[0m[�[0m�[31merror�[0m] �[0m�[0mFailed tests:�[0m
2022-05-06T21:20:15.6791337Z �[0m[�[0m�[31merror�[0m] �[0m�[0m	org.apache.spark.sql.TPCDSV1_4_PlanStabilitySuite�[0m

Could you check the followings, @vli-databricks ?

2022-05-06T20:27:41.3079912Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m- check simplified (tpcds-v1.4/q4) *** FAILED *** (756 milliseconds)�[0m�[0m
2022-05-06T20:27:41.3080439Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  Plans did not match:�[0m�[0m
2022-05-06T20:27:41.3081161Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  last approved simplified plan: /home/runner/work/spark/spark/sql/core/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q4/simplified.txt�[0m�[0m
2022-05-06T20:27:41.3082161Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  last approved explain plan: /home/runner/work/spark/spark/sql/core/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q4/explain.txt�[0m�[0m
2022-05-06T20:27:41.7834499Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m- check simplified (tpcds-v1.4/q5) *** FAILED *** (434 milliseconds)�[0m�[0m
2022-05-06T20:27:41.7835431Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  Plans did not match:�[0m�[0m
2022-05-06T20:27:41.7836666Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  last approved simplified plan: /home/runner/work/spark/spark/sql/core/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q5/simplified.txt�[0m�[0m
2022-05-06T20:27:41.7837723Z �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  last approved explain plan: /home/runner/work/spark/spark/sql/core/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q5/explain.txt�[0m�[0m

@vitaliili-db
Copy link
Contributor Author

@dongjoon-hyun yes, I was digging into that but the output plans extracted from logs are exactly the same (there is an extra new line in approved but it seems to be ignored). My local run succeeds and regenerating golden files also does not change anything. It might be a flake or unseen character or something else. I filed a separate issue.

Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

The test failures are not related to the changes. +1 to merge it to 3.2.

@dongjoon-hyun
Copy link
Member

Feel free to proceed according to your decision, @MaxGekk ~

MaxGekk pushed a commit that referenced this pull request May 12, 2022
### What changes were proposed in this pull request?

This PR removes extra curly bracket from debug string for Decimal type in SQL.

This is a backport from master branch. Commit: 165ce4e

### Why are the changes needed?

Typo in error messages of decimal overflow.

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

No

### How was this patch tested?

By running tests:
```
$ build/sbt "sql/testOnly"
```

Closes #36458 from vli-databricks/SPARK-39060-3.2.

Authored-by: Vitalii Li <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
@MaxGekk
Copy link
Member

MaxGekk commented May 12, 2022

+1, LGTM. Merged to 3.2.
Thank you, @vli-databricks, and @dongjoon-hyun for review.

@MaxGekk MaxGekk closed this May 12, 2022
kazuyukitanimura pushed a commit to kazuyukitanimura/spark that referenced this pull request Aug 10, 2022
### What changes were proposed in this pull request?

This PR removes extra curly bracket from debug string for Decimal type in SQL.

This is a backport from master branch. Commit: apache@165ce4e

### Why are the changes needed?

Typo in error messages of decimal overflow.

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

No

### How was this patch tested?

By running tests:
```
$ build/sbt "sql/testOnly"
```

Closes apache#36458 from vli-databricks/SPARK-39060-3.2.

Authored-by: Vitalii Li <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
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