codetyri0n commented on PR #18174: URL: https://github.com/apache/datafusion/pull/18174#issuecomment-3450060270
> Oh I'm a little confused now; so the initial state of the PR was it **didn't** support optional 2nd arg and thats how it differed from DataFusion version which mistakenly does; DataFusion version was fixed to accept only one argument by #18265. But now this PR **does** implement optional 2nd arg for Spark (so things are essentially flipped from initial state). > > Could we have some documentation both in the code and in the PR body to highlight this? I'll try take a look at the code again now since I need to reframe my view of what's being added in this PR. to summarize the changes: - since the objective was to have a spark compatible crate and with Sail's implementation that allowed to us to do so I thought it would be better to incorporate the second argument too along with decimal support. - there was earlier confusion around what result type the function was supposed to return - which was not Int for all cases. Imo we can either - revert/comment the changes for second arg support as one of the test cases is failing, file a separate issue for it and introduce it as a second PR... OR go ahead fixing it and take due action. -- 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]
