Re: [PR] feat: Wire DataFusion function Claude skill and `csc` [datafusion-comet]

2026-05-15 Thread via GitHub 


comphead merged PR #4337:
URL: https://github.com/apache/datafusion-comet/pull/4337


-- 
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]

Re: [PR] feat: Wire DataFusion function Claude skill and `csc` [datafusion-comet]

2026-05-15 Thread via GitHub 


comphead commented on code in PR #4337:
URL: https://github.com/apache/datafusion-comet/pull/4337#discussion_r3251000743


##
spark/src/test/resources/sql-tests/expressions/math/csc.sql:
##
@@ -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.
+
+statement
+CREATE TABLE test_csc(d double) USING parquet
+
+statement
+INSERT INTO test_csc VALUES (0.0), (-0.0), (1.5707963267948966), 
(-1.5707963267948966), (3.141592653589793), (NULL), (cast('NaN' as double)), 
(cast('Infinity' as double)), (cast('-Infinity' as double))

Review Comment:
   Thanks for the review. the skill has min/max values for int or bigints. the 
thing here is skill add tests depending on signature in spark and defined as 
double in input
   
   ```
   case class Csc(child: Expression)
 extends UnaryMathExpression((x: Double) => 1 / math.sin(x), "CSC")
   
 override def inputTypes: Seq[AbstractDataType] = Seq(DoubleType)
   ```
   
   So only doubles edge cases are checked. This is just a first function 
implemented with the skill, so I expect the skill will be improved with time



-- 
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]

Re: [PR] feat: Wire DataFusion function Claude skill and `csc` [datafusion-comet]

2026-05-15 Thread via GitHub 


kazuyukitanimura commented on code in PR #4337:
URL: https://github.com/apache/datafusion-comet/pull/4337#discussion_r3250869193


##
spark/src/test/resources/sql-tests/expressions/math/csc.sql:
##
@@ -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.
+
+statement
+CREATE TABLE test_csc(d double) USING parquet
+
+statement
+INSERT INTO test_csc VALUES (0.0), (-0.0), (1.5707963267948966), 
(-1.5707963267948966), (3.141592653589793), (NULL), (cast('NaN' as double)), 
(cast('Infinity' as double)), (cast('-Infinity' as double))

Review Comment:
   Could you add, max, min, min_positive(subnormal) numbers here (and in the 
skill), please?



-- 
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]

[PR] feat: Wire DataFusion function Claude skill and `csc` [datafusion-comet]

2026-05-14 Thread via GitHub 


comphead opened a new pull request, #4337:
URL: https://github.com/apache/datafusion-comet/pull/4337

   
   ## Which issue does this PR close?
   
   
   
   Part #4150
   
   ## Rationale for this change
   
 Spark `csc(x) = 1 / sin(x)` was unsupported in Comet, falling back to 
Spark. The upstream `datafusion-spark` crate (53.1.0) ships a Spark-compatible 
`SparkCsc` implementation, so we can wire it
 directly with no native Rust written in this repo.
   
 This PR also adds a new Claude Code skill that codifies the wiring 
workflow used here, so future contributors can apply the same recipe 
consistently.
   
 ## What changes are included in this PR?
   
 **Wire Spark `Csc` to `datafusion-spark`:**
   
 - `spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala` — 
register `classOf[Csc] -> CometScalarFunction("csc")` in `mathExpressions`.
 - `native/core/src/execution/jni_api.rs` — register `SparkCsc` in 
`register_datafusion_spark_function`.
 - `docs/source/user-guide/latest/expressions.md` — add `Csc` to the Math 
Expressions table.
 - `docs/source/contributor-guide/spark_expressions_support.md` — flip 
`csc` checkbox to supported.
   
 Upstream function: 
`datafusion_spark::function::math::trigonometry::SparkCsc`. Wiring follows 
Pattern B (one-line Scala passthrough + one-line UDF registration).
   
 **New Claude Code skill `wire-datafusion-function`:**
   
 - `.claude/skills/wire-datafusion-function/SKILL.md` — project-level skill 
that walks contributors through wiring an existing DataFusion or 
`datafusion-spark` function into Comet. Documents the three
 wiring patterns (passthrough, UDF registration, custom serde with 
restrictions), the SQL-test coverage requirements (parquet-backed inputs, 
NULLs, FP edge cases, integer/decimal min/max), and the
 hand-curated docs that must be updated. Complements the existing 
`implement-comet-expression` skill — used here to scaffold the csc wiring.
   
   
   


-- 
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]