Skip to content

[SPARK-40136][SQL] Fix the fragment of SQL query contexts - #37566

Closed
MaxGekk wants to merge 4 commits into
apache:masterfrom
MaxGekk:fix-fragment
Closed

[SPARK-40136][SQL] Fix the fragment of SQL query contexts#37566
MaxGekk wants to merge 4 commits into
apache:masterfrom
MaxGekk:fix-fragment

Conversation

@MaxGekk

@MaxGekkMaxGekk commented Aug 18, 2022

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

In the PR, I propose to change implementation of fragment in SQLQueryContext to get the fragment correctly. Before the changes, fragment misses the last char because originStopIndex.get points to the last char of fragments but substring() gets a sub-string up to endIndex - 1.

Why are the changes needed?

It fixes a bug.

Does this PR introduce any user-facing change?

Yes.

How was this patch tested?

By running new test suite:

$ build/sbt "test:testOnly *SQLQueryContextSuite"
$ build/sbt "test:testOnly *TreeNodeSuite"
$ build/sbt "test:testOnly *QueryExecutionAnsiErrorsSuite"

@MaxGekk

Copy link
Copy Markdown
MemberAuthor

@cloud-fan@HyukjinKwon Could you review this bug fix, please.


class SQLQueryContextSuite extends SparkFunSuite with SQLHelper {

test("SPARK-40136: the length of the fragment in query context") {

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.

this basically tests String.substring... can we have an end-to-end test?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@cloud-fan The failed test DIVIDE_BY_ZERO: can't divide an integer by zero *** FAILED *** is an end-to-end test actually. Let me fix it.

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.

Great! Can we remove this test suite then? The code we are testing is simply sqlText.get.substring(originStartIndex.get, originStopIndex.get + 1)

@MaxGekkMaxGekkAug 18, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I wouldn't remove this unit tests because:

  1. Current implementation of fragment is not only substring(). It is a combination of isValid + substring.
  2. The end-to-end tests don't check corner cases.

SQLQueryContext has poor test coverage currently. Instead of to hope that end-to-end tests guarantee its correctness, I would add more simple unit tests to SQLQueryContext.

@MaxGekk

Copy link
Copy Markdown
MemberAuthor

The failure of ImageFileFormatSuite is not related to the PR, I believe. @cloud-fan Could you take a look at this PR one more time, please.

@dongjoon-hyundongjoon-hyun 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.

+1, LGTM.

@MaxGekk

Copy link
Copy Markdown
MemberAuthor

Merging to master. Thank you, @cloud-fan@dongjoon-hyun for review.

Sign up for freeto 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.

3 participants

@MaxGekk@cloud-fan@dongjoon-hyun