Skip to content

[SPARK-49121][PROTOBUF][TESTS][FOLLOWUP] Change to dynamically calculate ExpectedContext#stop in test case "SPARK-49121: from_protobuf and to_protobuf SQL functions"#47855

Closed
LuciferYang wants to merge 5 commits into
apache:masterfrom
LuciferYang:SPARK-49121-FOLLOWUP

Conversation

@LuciferYang

@LuciferYang LuciferYang commented Aug 23, 2024

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Currently, in the test case "SPARK-49121: from_protobuf and to_protobuf SQL functions", for the test case where the fragment contains the testFileDescFile variable, we also use an absolute position to define ExpectedContext#stop.

However, since the content of testFileDescFile is related to both the build tool and the execution directory of the test case, its length is not fixed.

For example, the maven daily test failed

- SPARK-49121: from_protobuf and to_protobuf SQL functions *** FAILED ***
  150 did not equal 153 Invalid stopIndex of a query context. Actual:SQLQueryContext(Some(3),Some(2),Some(10),Some(150),Some(
  SELECT
    to_protobuf(complex_struct, 42, '/home/runner/work/spark/spark/connector/protobuf/target/generated-test-sources/functions_suite.desc', map())
  FROM protobuf_test_table
  ),None,None) (SparkFunSuite.scala:376)

and when I run the test locally using sbt, it also failed:

build/sbt "protobuf/testOnly org.apache.spark.sql.protobuf.ProtobufFunctionsSuite"
[info] - SPARK-49121: from_protobuf and to_protobuf SQL functions *** FAILED *** (248 milliseconds)
[info]   165 did not equal 153 Invalid stopIndex of a query context. Actual:SQLQueryContext(Some(3),Some(2),Some(10),Some(165),Some(
[info]   SELECT
[info]     to_protobuf(complex_struct, 42, '/Users/yangjie01/SourceCode/git/spark-sbt/connector/protobuf/target/generated-test-sources/descriptor-set-sbt.desc', map())
[info]   FROM protobuf_test_table
[info]   ),None,None) (SparkFunSuite.scala:376)

So this PR changes ExpectedContext#stop to a relative definition: stop = (start - 1) + prefixLength + fileNameLength + suffixLength

For example:

...
queryContext = Array(ExpectedContext(
          fragment = s"to_protobuf(complex_struct, 42, '$testFileDescFile', map())",
          start = 10,
          stop = 153))
...
(start - 1) = 10 -1 = 9
prefixLength = "to_protobuf(complex_struct, 42, '".length = 33
suffixLength = "', map())".length = 9

So the stop definition will change from stop = 153 to stop = 9 + 33 + fileNameLength + 9, similar change have been made to other cases as well.

Why are the changes needed?

Make the test more robust and fix the maven daily test.

Does this PR introduce any user-facing change?

No

How was this patch tested?

  • Pass GitHub Actions
  • locally test:

maven:

build/mvn clean install -pl connector/protobuf -am -DskipTests
build/mvn test -pl connector/protobuf -Dtest=none -DwildcardSuites=org.apache.spark.sql.protobuf.ProtobufFunctionsSuite
Run completed in 10 seconds, 719 milliseconds.
Total number of tests run: 40
Suites: completed 2, aborted 0
Tests: succeeded 40, failed 0, canceled 0, ignored 0, pending 0
All tests passed.

sbt:

build/sbt clean "protobuf/testOnly org.apache.spark.sql.protobuf.ProtobufFunctionsSuite"
[info] Run completed in 11 seconds, 786 milliseconds.
[info] Total number of tests run: 40
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 40, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.

image

maven tests passed

Was this patch authored or co-authored using generative AI tooling?

No

stop = 153))
// The subsequent test scenarios also follow the following rules to define `stop`:
// stop = (start - 1) + prefixLength + fileNameLength + suffixLength
stop = 9 + 33 + fileNameLength + 9))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@itholic If you haven't started investigating yet, could you please help check if this fix is feasible? Thanks
also cc @HyukjinKwon

@LuciferYang LuciferYang changed the title [SPARK-49121][TESTS][FOLLOWUP] Change to dynamically calculate ExpectedContext#stop in test case "SPARK-49121: from_protobuf and to_protobuf SQL functions" [SPARK-49121][PROTOBUF][TESTS][FOLLOWUP] Change to dynamically calculate ExpectedContext#stop in test case "SPARK-49121: from_protobuf and to_protobuf SQL functions" Aug 23, 2024
@github-actions github-actions Bot added the INFRA label Aug 23, 2024
Comment thread .github/workflows/build_and_test.yml Outdated
@github-actions github-actions Bot removed the INFRA label Aug 23, 2024
@LuciferYang LuciferYang marked this pull request as ready for review August 23, 2024 11:37
@LuciferYang

Copy link
Copy Markdown
Contributor Author

Will close this one due to #47859 seems already fix this issue.

@LuciferYang LuciferYang deleted the SPARK-49121-FOLLOWUP branch May 2, 2025 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants