parthchandra commented on code in PR #1569:
URL: https://github.com/apache/datafusion-comet/pull/1569#discussion_r2013108125
##########
spark/src/test/scala/org/apache/comet/CometSparkSessionExtensionsSuite.scala:
##########
@@ -27,24 +27,24 @@ class CometSparkSessionExtensionsSuite extends
CometTestBase {
import CometSparkSessionExtensions._
- test("isCometEnabled") {
+ test("isCometLoaded") {
val conf = new SQLConf
conf.setConfString(CometConf.COMET_ENABLED.key, "false")
- assert(!isCometEnabled(conf))
+ assert(!isCometLoaded(conf))
Review Comment:
> Thanks, I thought about it but since both version of `isCometEnabled`
would be still accessible, especially due to
>
> ```
> import CometSparkSessionExtensions._
> ```
>
> , this may cause future issues for using `isCometEnabled` like adding new
tests. I am ok with `CometSparkSessionExtensions.isCometEnabled` but changing
the name entirely is more future proof? WDTY @parthchandra @comphead Also
`CometSparkSessionExtensions.isCometEnabled` is checking `NativeBase.isLoaded`,
so I think `isCometLoaded` makes sense
It used to be CometSparkSessionExtensions.isCometEnabled and was changed
here:
https://github.com/apache/datafusion-comet/commit/badbd376898cb4f80b5136a90697019ad11a2e90
I'm okay with the new name though.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
