You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kazuyukitanimura (via GitHub)" <gi...@apache.org> on 2024/05/29 17:09:04 UTC
[PR] build: Enable comet tests with spark-4.0 profile [datafusion-comet]
kazuyukitanimura opened a new pull request, #493:
URL: https://github.com/apache/datafusion-comet/pull/493
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Part of https://github.com/apache/datafusion-comet/issues/372
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
To be ready for Spark 4.0
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
This PR enables the comet tests with the spark-4.0 profile
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Tests with the spark-4.0 profile now should pass. (But Spark tests do not yet)
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625133537
##########
common/src/main/spark-4.0/org/apache/comet/shims/ShimCometConf.scala:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.shims
+
+trait ShimCometConf {
+ protected val COMET_SCHEMA_EVOLUTION_ENABLED_DEFAULT = true
+ protected val COMET_ANSI_MODE_ENABLED_DEFAULT = true
Review Comment:
Also see https://github.com/apache/datafusion-comet/pull/509 to allow us to fall back per function if ANSI is not supported yet and https://github.com/apache/datafusion-comet/issues/313 where we are tracking all ANSI work
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625107770
##########
common/src/main/scala/org/apache/comet/CometConf.scala:
##########
@@ -382,12 +384,13 @@ object CometConf {
.createWithDefault(Seq("Range,InMemoryTableScan"))
val COMET_ANSI_MODE_ENABLED: ConfigEntry[Boolean] = conf("spark.comet.ansi.enabled")
+ .internal()
.doc(
"Comet does not respect ANSI mode in most cases and by default will not accelerate " +
"queries when ansi mode is enabled. Enable this setting to test Comet's experimental " +
"support for ANSI mode. This should not be used in production.")
.booleanConf
- .createWithDefault(false)
+ .createWithDefault(COMET_ANSI_MODE_ENABLED_DEFAULT)
Review Comment:
We may need to mention it is enabled by default for Spark 4.0 now. It could produce incorrect results so we better don't make it implicitly enabled.
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#issuecomment-2146062558
Friendly ping @viirya @andygrove @comphead @huaxingao
I plan to merge this unless I hear back.
@parthchandra already approved.
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#issuecomment-2138375649
cc @viirya @andygrove @comphead @huaxingao
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625073733
##########
docs/source/user-guide/configs.md:
##########
@@ -23,7 +23,6 @@ Comet provides the following configuration settings.
| Config | Description | Default Value |
|--------|-------------|---------------|
-| spark.comet.ansi.enabled | Comet does not respect ANSI mode in most cases and by default will not accelerate queries when ansi mode is enabled. Enable this setting to test Comet's experimental support for ANSI mode. This should not be used in production. | false |
Review Comment:
This highlights that our configs vary depending on the version we build against and we currently only publish one version of our configs.
Ideally we should generate configs per spark version and publish all of them.
##########
docs/source/user-guide/configs.md:
##########
@@ -23,7 +23,6 @@ Comet provides the following configuration settings.
| Config | Description | Default Value |
|--------|-------------|---------------|
-| spark.comet.ansi.enabled | Comet does not respect ANSI mode in most cases and by default will not accelerate queries when ansi mode is enabled. Enable this setting to test Comet's experimental support for ANSI mode. This should not be used in production. | false |
Review Comment:
I will file an issue for this
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625095678
##########
common/src/main/spark-3.x/org/apache/comet/shims/ShimCometConf.scala:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.shims
+
+trait ShimCometConf {
+ protected val COMET_SCHEMA_EVOLUTION_ENABLED_DEFAULT = false
Review Comment:
nit: do we need a trailing _DEFAULT in the 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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1621366500
##########
spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala:
##########
@@ -1125,7 +1123,9 @@ abstract class ParquetReadSuite extends CometTestBase {
}
test("row group skipping doesn't overflow when reading into larger type") {
- assume(isSpark34Plus)
+ // Spark 4.0 no longer fails for widening types
+ // https://github.com/apache/spark/commit/3361f25dc0ff6e5233903c26ee105711b79ba967
+ assume(isSpark34Plus && !isSpark34Plus)
Review Comment:
Yes, you are right. Thank you!
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625093195
##########
common/src/main/spark-4.0/org/apache/comet/shims/ShimCometConf.scala:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.shims
+
+trait ShimCometConf {
+ protected val COMET_SCHEMA_EVOLUTION_ENABLED_DEFAULT = true
+ protected val COMET_ANSI_MODE_ENABLED_DEFAULT = true
Review Comment:
I am thinking to use Spark-4.0 as a test bed for ANSI readiness. I will send another PR for enabling Spark tests and if I find any incompatibility, I will disable them per function.
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625120541
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -2201,7 +2201,8 @@ object QueryPlanSerde extends Logging with ShimQueryPlanSerde with CometExprShim
}
def nullIfWhenPrimitive(expression: Expression): Expression = if (isPrimitive(expression)) {
- new NullIf(expression, Literal.default(expression.dataType)).child
+ val zero = Literal.default(expression.dataType)
+ If(EqualTo(expression, zero), Literal.create(null, expression.dataType), expression)
Review Comment:
`NullIf` implementation was changed to use `With` that Comet does not support https://github.com/apache/spark/pull/43623
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625084169
##########
docs/source/user-guide/configs.md:
##########
@@ -23,7 +23,6 @@ Comet provides the following configuration settings.
| Config | Description | Default Value |
|--------|-------------|---------------|
-| spark.comet.ansi.enabled | Comet does not respect ANSI mode in most cases and by default will not accelerate queries when ansi mode is enabled. Enable this setting to test Comet's experimental support for ANSI mode. This should not be used in production. | false |
Review Comment:
Thanks, I plan to associate this config with `spark.sql.ansi.enabled`. So when Spark enables ANSI, so does Comet
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1621337892
##########
spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala:
##########
@@ -1125,7 +1123,9 @@ abstract class ParquetReadSuite extends CometTestBase {
}
test("row group skipping doesn't overflow when reading into larger type") {
- assume(isSpark34Plus)
+ // Spark 4.0 no longer fails for widening types
+ // https://github.com/apache/spark/commit/3361f25dc0ff6e5233903c26ee105711b79ba967
+ assume(isSpark34Plus && !isSpark34Plus)
Review Comment:
Did you mean `assume(isSpark34Plus && !isSpark40Plus)` ?
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#issuecomment-2146269223
Thank you @viirya @andygrove @comphead @parthchandra
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625078324
##########
common/src/main/spark-4.0/org/apache/comet/shims/ShimCometConf.scala:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.shims
+
+trait ShimCometConf {
+ protected val COMET_SCHEMA_EVOLUTION_ENABLED_DEFAULT = true
+ protected val COMET_ANSI_MODE_ENABLED_DEFAULT = true
Review Comment:
I suppose we have to do this otherwise all plans will currently fall back to Spark because ANSI mode is enabled by default in Spark 4.
We should really only fall back to Spark for expressions that are actually affected by ANSI support rather than fall back for everything.
Perhaps we could add a note to the documentation explaining that ANSI mode in Spark 4 is not fully compatible yet.
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625108499
##########
common/src/main/spark-3.x/org/apache/comet/shims/ShimCometConf.scala:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.shims
+
+trait ShimCometConf {
+ protected val COMET_SCHEMA_EVOLUTION_ENABLED_DEFAULT = false
Review Comment:
I would say so as we have `COMET_SCHEMA_EVOLUTION_ENABLED` in conf. Also this is default so that users can overwrite.
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura merged PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#issuecomment-2146189000
> I tihnk we should add a note to the documentation saying that we support Spark 4 now and also point out that ANSI mode is not yet compatible. I think it would be fine to do that as a follow-up PR.
Thank you @andygrove I plan to send a few more PRs on Spark-4.0
Yes, I will document things about Spark-4.0
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625113107
##########
common/src/main/scala/org/apache/comet/CometConf.scala:
##########
@@ -382,12 +384,13 @@ object CometConf {
.createWithDefault(Seq("Range,InMemoryTableScan"))
val COMET_ANSI_MODE_ENABLED: ConfigEntry[Boolean] = conf("spark.comet.ansi.enabled")
+ .internal()
.doc(
"Comet does not respect ANSI mode in most cases and by default will not accelerate " +
"queries when ansi mode is enabled. Enable this setting to test Comet's experimental " +
"support for ANSI mode. This should not be used in production.")
.booleanConf
- .createWithDefault(false)
+ .createWithDefault(COMET_ANSI_MODE_ENABLED_DEFAULT)
Review Comment:
Right, Spark 4.0 support is not ready at all. I will send follow up PRs
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625074176
##########
common/src/main/spark-4.0/org/apache/comet/shims/ShimCometConf.scala:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.shims
+
+trait ShimCometConf {
+ protected val COMET_SCHEMA_EVOLUTION_ENABLED_DEFAULT = true
+ protected val COMET_ANSI_MODE_ENABLED_DEFAULT = true
Review Comment:
Do we really want to enable this by default when we know that we are not compatible yet?
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625108088
##########
common/src/main/spark-4.0/org/apache/comet/shims/ShimCometConf.scala:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.shims
+
+trait ShimCometConf {
+ protected val COMET_SCHEMA_EVOLUTION_ENABLED_DEFAULT = true
+ protected val COMET_ANSI_MODE_ENABLED_DEFAULT = true
Review Comment:
You need it to be enabled for Spark tests, isn't?
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625112152
##########
common/src/main/scala/org/apache/comet/CometConf.scala:
##########
@@ -382,12 +384,13 @@ object CometConf {
.createWithDefault(Seq("Range,InMemoryTableScan"))
val COMET_ANSI_MODE_ENABLED: ConfigEntry[Boolean] = conf("spark.comet.ansi.enabled")
+ .internal()
Review Comment:
As discussed in https://github.com/apache/datafusion-comet/pull/493#discussion_r1625073733
doc is not per profile yet. By making it internal, doc will ignore 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
Re: [PR] build: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625109403
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -2201,7 +2201,8 @@ object QueryPlanSerde extends Logging with ShimQueryPlanSerde with CometExprShim
}
def nullIfWhenPrimitive(expression: Expression): Expression = if (isPrimitive(expression)) {
- new NullIf(expression, Literal.default(expression.dataType)).child
+ val zero = Literal.default(expression.dataType)
+ If(EqualTo(expression, zero), Literal.create(null, expression.dataType), expression)
Review Comment:
?
--
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: Enable comet tests with spark-4.0 profile [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #493:
URL: https://github.com/apache/datafusion-comet/pull/493#discussion_r1625107081
##########
common/src/main/scala/org/apache/comet/CometConf.scala:
##########
@@ -382,12 +384,13 @@ object CometConf {
.createWithDefault(Seq("Range,InMemoryTableScan"))
val COMET_ANSI_MODE_ENABLED: ConfigEntry[Boolean] = conf("spark.comet.ansi.enabled")
+ .internal()
Review Comment:
Why making it internal?
--
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