andygrove commented on code in PR #6069: URL: https://github.com/apache/datafusion-comet/pull/6069#discussion_r4065766809
########## spark/src/test/resources/sql-tests/windows/sliding_integer_sum.sql: ########## @@ -0,0 +1,129 @@ +-- 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. + +-- Config: spark.sql.adaptive.enabled=false +-- Config: spark.sql.ansi.enabled=false +-- Config: spark.comet.operator.WindowExec.allowIncompatible=false + +-- Spark needs constant folding for PRECEDING bounds. Aggregate inputs remain columns. +statement +SET spark.sql.optimizer.excludedRules= Review Comment: Thanks for the comment explaining why this is here, it made the intent easy to follow. One concern though. `CometSqlFileTestSuite` excludes `ConstantFolding` on purpose, and appends that exclusion after each file's own configs so that a header cannot override it. Clearing it with a `SET` statement turns folding back on for every query in the rest of the file, including the plain `query` entries that assert native coverage. This is the only fixture in the repo that does this. `window_functions.sql` lines 198 to 203 hit the same `ROWS ... N PRECEDING` problem and took the other route, keeping SQL fixtures to bounds that parse directly and covering `N PRECEDING` in `CometWindowExecSuite` through the DataFrame API. Could we follow that convention here? The `RANGE ... 1 PRECEDING` cases already work unfolded, as `window_functions.sql` line 226 shows, so only the `ROWS` ones would need to move and the `SET` could go away entirely. If you think the fixture really does need folding, could we add a supported file-level directive for it instead, so the harness stays in control of what it guarantees? ########## spark/src/test/scala/org/apache/comet/exec/CometWindowExecSuite.scala: ########## @@ -528,6 +538,8 @@ class CometWindowExecSuite extends CometTestBase { test("Windows support") { Seq("true", "false").foreach(aqeEnabled => withSQLConf( + // This native coverage matrix includes legacy sliding integral sums. + SQLConf.ANSI_ENABLED.key -> "false", Review Comment: I follow why this is needed. The default profile is Spark 4.1.3 where ANSI is on, so without this the sliding sums in this matrix would now fall back and `checkSparkAnswerAndOperator` would fail its native coverage assertion. The side effect is that this matrix stops exercising ANSI at all, and it covers a good deal more than sums. Would routing the sum assertions through `checkSlidingIntegralSum` work here, the way the three tests below do, so the matrix keeps running in both modes? If pinning really is the cleaner option, could the comment mention that the matrix no longer covers ANSI? As written it reads as being only about legacy sums, and the next person may not realise the broader coverage went with it. ########## spark/src/test/resources/sql-tests/windows/sliding_integer_sum.sql: ########## @@ -0,0 +1,129 @@ +-- 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. + +-- Config: spark.sql.adaptive.enabled=false +-- Config: spark.sql.ansi.enabled=false +-- Config: spark.comet.operator.WindowExec.allowIncompatible=false Review Comment: Is `spark.comet.operator.WindowExec.allowIncompatible=false` doing anything here? `CometWindowExec` does not declare a support level, and the only other fixture using that key, `lag_lead.sql`, sets it to `true`. If this is just documenting the default, it may be worth dropping so that it does not get copied into future fixtures as boilerplate. -- 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]
