You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "snmvaughan (via GitHub)" <gi...@apache.org> on 2024/05/07 17:30:47 UTC
[PR] build: Add scala-version to matrix [datafusion-comet]
snmvaughan opened a new pull request, #396:
URL: https://github.com/apache/datafusion-comet/pull/396
## Which issue does this PR close?
Closes https://github.com/apache/datafusion-comet/issues/388
## Rationale for this change
scalafix check will fail when running Scala 2.13, but is skipped for Scala 2.12.
## What changes are included in this PR?
Add testing of Scala 2.13 along with 2.12 by adding `scala-version` to the matrix. Explicitly exclude combinations of Spark `3.2` and Scala `2.13`.
## How are these changes tested?
Local repo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] build: Add scala-version to matrix [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #396:
URL: https://github.com/apache/datafusion-comet/pull/396#discussion_r1593206963
##########
spark/src/test/scala/org/apache/comet/exec/CometColumnarShuffleSuite.scala:
##########
@@ -587,7 +587,7 @@ abstract class CometColumnarShuffleSuite extends CometTestBase with AdaptiveSpar
}
test("columnar shuffle on nested array") {
- Seq("false", "true").foreach { execEnabled =>
+ Seq("false", "true").foreach { _ =>
Review Comment:
looks like this needs to be addressed in a follow on PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] build: Add scala-version to matrix [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #396:
URL: https://github.com/apache/datafusion-comet/pull/396#issuecomment-2099324247
> I expected these errors, which is why I wanted to get Scala 2.13 into the pipeline in the first place. The real question is do we want to add a bunch of unrelated fixes into this PR?
If we don't fix them here, we will have failed pipelines. I think we should not merge a PR which has failed pipelines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] build: Add scala-version to matrix [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #396:
URL: https://github.com/apache/datafusion-comet/pull/396#discussion_r1593206071
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -867,26 +867,27 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
- private def castFallbackTestTimezone(
- input: DataFrame,
- toType: DataType,
- expectedMessage: String): Unit = {
- withTempPath { dir =>
- val data = roundtripParquet(input, dir).coalesce(1)
- data.createOrReplaceTempView("t")
-
- withSQLConf(
- (SQLConf.ANSI_ENABLED.key, "false"),
- (CometConf.COMET_CAST_ALLOW_INCOMPATIBLE.key, "true"),
- (SQLConf.SESSION_LOCAL_TIMEZONE.key, "America/Los_Angeles")) {
- val df = data.withColumn("converted", col("a").cast(toType))
- df.collect()
- val str =
- new ExtendedExplainInfo().generateExtendedInfo(df.queryExecution.executedPlan)
- assert(str.contains(expectedMessage))
- }
- }
- }
+ // TODO Commented out to work around scalafix since this is currently unused.
Review Comment:
Thank you. I will see if this is still needed and follow up to use or remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] build: Add scala-version to matrix [datafusion-comet]
Posted by "snmvaughan (via GitHub)" <gi...@apache.org>.
snmvaughan commented on code in PR #396:
URL: https://github.com/apache/datafusion-comet/pull/396#discussion_r1592911593
##########
.github/workflows/pr_build.yml:
##########
@@ -82,11 +83,14 @@ jobs:
java_version: [8, 11, 17]
test-target: [java]
spark-version: ['3.2', '3.3']
+ scala-version: ['2.12', '2.13']
exclude:
- java_version: 17
spark-version: '3.2'
- java_version: 11
spark-version: '3.2'
+ - spark-version: '3.2'
+ scala-version: '2.13'
fail-fast: false
name: ${{ matrix.os }}/java ${{ matrix.java_version }}-spark-${{matrix.spark-version}}/${{ matrix.test-target }}
Review Comment:
I missed that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] build: Add scala-version to matrix [datafusion-comet]
Posted by "snmvaughan (via GitHub)" <gi...@apache.org>.
snmvaughan commented on code in PR #396:
URL: https://github.com/apache/datafusion-comet/pull/396#discussion_r1593242920
##########
spark/src/test/scala/org/apache/comet/exec/CometColumnarShuffleSuite.scala:
##########
@@ -587,7 +587,7 @@ abstract class CometColumnarShuffleSuite extends CometTestBase with AdaptiveSpar
}
test("columnar shuffle on nested array") {
- Seq("false", "true").foreach { execEnabled =>
+ Seq("false", "true").foreach { _ =>
Review Comment:
Good point. That `execEnabled` wasn't being used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] build: Add scala-version to matrix [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #396:
URL: https://github.com/apache/datafusion-comet/pull/396#discussion_r1592862382
##########
.github/workflows/pr_build.yml:
##########
@@ -82,11 +83,14 @@ jobs:
java_version: [8, 11, 17]
test-target: [java]
spark-version: ['3.2', '3.3']
+ scala-version: ['2.12', '2.13']
exclude:
- java_version: 17
spark-version: '3.2'
- java_version: 11
spark-version: '3.2'
+ - spark-version: '3.2'
+ scala-version: '2.13'
fail-fast: false
name: ${{ matrix.os }}/java ${{ matrix.java_version }}-spark-${{matrix.spark-version}}/${{ matrix.test-target }}
Review Comment:
Add scala-version to the pipeline name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] build: Add scala-version to matrix [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #396:
URL: https://github.com/apache/datafusion-comet/pull/396#issuecomment-2099495728
Merged. Thanks @snmvaughan @andygrove
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] build: Add scala-version to matrix [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya merged PR #396:
URL: https://github.com/apache/datafusion-comet/pull/396
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] build: Add scala-version to matrix [datafusion-comet]
Posted by "snmvaughan (via GitHub)" <gi...@apache.org>.
snmvaughan commented on PR #396:
URL: https://github.com/apache/datafusion-comet/pull/396#issuecomment-2099121918
> Do you want to fix these scala format errors together? Otherwise these new pipelines are not passed.
I expected these errors, which is why I wanted to get Scala 2.13 into the pipeline in the first place. The real question is do we want to add a bunch of unrelated fixes into this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] build: Add scala-version to matrix [datafusion-comet]
Posted by "snmvaughan (via GitHub)" <gi...@apache.org>.
snmvaughan commented on code in PR #396:
URL: https://github.com/apache/datafusion-comet/pull/396#discussion_r1593075945
##########
spark/src/test/scala/org/apache/comet/CometCastSuite.scala:
##########
@@ -867,26 +867,27 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
- private def castFallbackTestTimezone(
- input: DataFrame,
- toType: DataType,
- expectedMessage: String): Unit = {
- withTempPath { dir =>
- val data = roundtripParquet(input, dir).coalesce(1)
- data.createOrReplaceTempView("t")
-
- withSQLConf(
- (SQLConf.ANSI_ENABLED.key, "false"),
- (CometConf.COMET_CAST_ALLOW_INCOMPATIBLE.key, "true"),
- (SQLConf.SESSION_LOCAL_TIMEZONE.key, "America/Los_Angeles")) {
- val df = data.withColumn("converted", col("a").cast(toType))
- df.collect()
- val str =
- new ExtendedExplainInfo().generateExtendedInfo(df.queryExecution.executedPlan)
- assert(str.contains(expectedMessage))
- }
- }
- }
+ // TODO Commented out to work around scalafix since this is currently unused.
Review Comment:
@andygrove I commented this out thinking you still might want it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org