Re: [PR] fix: Make cast from float/double to decimal compatible with Spark [datafusion-comet]
leung-ming closed pull request #1915: fix: Make cast from float/double to decimal compatible with Spark URL: https://github.com/apache/datafusion-comet/pull/1915 -- 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] fix: Make cast from float/double to decimal compatible with Spark [datafusion-comet]
parthchandra commented on PR #1915: URL: https://github.com/apache/datafusion-comet/pull/1915#issuecomment-3169545796 @andygrove It does look like the creator of dragonbox is recommending we fork their code. It does create some bloat if we include it in Comet. Is there any other alternative? Maybe add it as an independent crate to datafusion-contrib? -- 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] fix: Make cast from float/double to decimal compatible with Spark [datafusion-comet]
leung-ming commented on PR #1915: URL: https://github.com/apache/datafusion-comet/pull/1915#issuecomment-3156012433 > Could this be done in the original crate? I understand that the original point of making the methods private is probably because the algorithm is intended specifically for int/float to string (not decimal) and also because the rust implementation appears to be for the purpose of testing. But if the reference implementation exposes the decimal interface then perhaps the original author might be okay with making the rust methods public too. Sorry for the long delay. Given that dragonbox looks not quite responteive, I opened an issue on schubfach_rs to discuss about it at https://github.com/blueglyph/schubfach_rs/issues/1 and I think what he say is reasonable. Maybe we should fork one or just use [ryu_floating_decimal](https://crates.io/crates/ryu_floating_decimal)? -- 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] fix: Make cast from float/double to decimal compatible with Spark [datafusion-comet]
leung-ming commented on PR #1915: URL: https://github.com/apache/datafusion-comet/pull/1915#issuecomment-3015182461 > > I am not implemented a new dragonbox, I just copy it, add 4 `pub` to expose the decimal interface. > > Could this be done in the original crate? I understand that the original point of making the methods private is probably because the algorithm is intended specifically for int/float to string (not decimal) and also because the rust implementation appears to be for the purpose of testing. But if the reference implementation exposes the decimal interface then perhaps the original author might be okay with making the rust methods public too. I try 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: [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] fix: Make cast from float/double to decimal compatible with Spark [datafusion-comet]
parthchandra commented on PR #1915: URL: https://github.com/apache/datafusion-comet/pull/1915#issuecomment-3006004050 @leung-ming do you think the dragonbox module you've implemented can be contributed back to [dragonbox](https://github.com/dtolnay/dragonbox)? I would feel more confident if someone with a better fundamental understanding of the algorithm reviewed your work. @andygrove the dragonbox crate has a Apache2-LLVM/Boost license. I think but am not sure, there should be no issue of compatibility with the Apache license? -- 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] fix: Make cast from float/double to decimal compatible with Spark [datafusion-comet]
leung-ming commented on PR #1915: URL: https://github.com/apache/datafusion-comet/pull/1915#issuecomment-3001405315 @parthchandra @andygrove I checked [ryu](https://crates.io/crates/ryu), [schubfach](https://crates.io/crates/schubfach) and [dragonbox](https://crates.io/crates/dragonbox), they don't exposes the floating point to decimal interfaces, only floating point to string interfaces there. Some one encounter the same problem and made [ryu_floating_decimal](https://crates.io/crates/ryu_floating_decimal), but I don't know it is still be maintained now. -- 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] fix: Make cast from float/double to decimal compatible with Spark [datafusion-comet]
leung-ming commented on PR #1915: URL: https://github.com/apache/datafusion-comet/pull/1915#issuecomment-3001437169 @parthchandra @andygrove I am going to do the following things those day, 1. check the issue in dragonbox repo. 2. compare it with the original c++ implementation and the paper. 3. check the rounding behavior is same with spark. If you decide not to use it, please tell me. -- 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] fix: Make cast from float/double to decimal compatible with Spark [datafusion-comet]
andygrove commented on PR #1915: URL: https://github.com/apache/datafusion-comet/pull/1915#issuecomment-3001390224 Thanks for finding the dragonbox crate @leung-ming. Is there a reason we must add the code to Comet rather than use it as a dependency? -- 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] fix: Make cast from float/double to decimal compatible with Spark [datafusion-comet]
parthchandra commented on PR #1915: URL: https://github.com/apache/datafusion-comet/pull/1915#issuecomment-3001310202 FWIW there is a - [Rust Schubfach create](https://docs.rs/schubfach/latest/schubfach/ ) -- 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] fix: Make cast from float/double to decimal compatible with Spark [datafusion-comet]
andygrove commented on code in PR #1915: URL: https://github.com/apache/datafusion-comet/pull/1915#discussion_r2161998053 ## native/spark-expr/src/conversion_funcs/schubfach.rs: ## @@ -0,0 +1,1517 @@ +// 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. + +// https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java Review Comment: I will start researching this today. -- 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] fix: Make cast from float/double to decimal compatible with Spark [datafusion-comet]
leung-ming commented on code in PR #1915: URL: https://github.com/apache/datafusion-comet/pull/1915#discussion_r2159314221 ## native/spark-expr/src/conversion_funcs/schubfach.rs: ## @@ -0,0 +1,1517 @@ +// 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. + +// https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java Review Comment: > Much of the code and comments in this file are copied from the JDK source code, which is GPL, so not compatible with the Apache license. May I claim I was copy from [here](https://github.com/c4f7fcce9cb06515/Schubfach/blob/master/todec/src/math/DoubleToDecimal.java) after checking any modifications from openjdk are dropped? -- 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] fix: Make cast from float/double to decimal compatible with Spark [datafusion-comet]
andygrove commented on code in PR #1915: URL: https://github.com/apache/datafusion-comet/pull/1915#discussion_r2159239834 ## native/spark-expr/src/conversion_funcs/schubfach.rs: ## @@ -0,0 +1,1517 @@ +// 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. + +// https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/math/DoubleToDecimal.java Review Comment: Much of the code and comments in this file are copied from the JDK source code, which is GPL, so not compatible with the Apache license. -- 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] fix: Make cast from float/double to decimal compatible with Spark [datafusion-comet]
codecov-commenter commented on PR #1915: URL: https://github.com/apache/datafusion-comet/pull/1915#issuecomment-2990042063 ## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/1915?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report Attention: Patch coverage is `0%` with `2 lines` in your changes missing coverage. Please review. > Project coverage is 32.75%. Comparing base [(`f09f8af`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/f09f8af64c6599255e116a376f4f008f2fd63b43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`fbd493f`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/fbd493fa3d0fd38d48a70ed6a86df7ca4df58080?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). > Report is 272 commits behind head on main. | [Files with missing lines](https://app.codecov.io/gh/apache/datafusion-comet/pull/1915?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [...scala/org/apache/comet/expressions/CometCast.scala](https://app.codecov.io/gh/apache/datafusion-comet/pull/1915?src=pr&el=tree&filepath=spark%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fcomet%2Fexpressions%2FCometCast.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3Bhcmsvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9jb21ldC9leHByZXNzaW9ucy9Db21ldENhc3Quc2NhbGE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion-comet/pull/1915?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Additional details and impacted files ```diff @@ Coverage Diff @@ ## main#1915 +/- ## = - Coverage 56.12% 32.75% -23.38% + Complexity 976 798 -178 = Files 119 130 +11 Lines 1174312744 +1001 Branches 2251 2405 +154 = - Hits 6591 4174 -2417 - Misses 4012 7598 +3586 + Partials 1140 972 -168 ``` [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/1915?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. -- 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]
