alamb commented on issue #14408: URL: https://github.com/apache/datafusion/issues/14408#issuecomment-2648260952
> > > [@findepi](https://github.com/findepi) do you mean we should relax the check to ignore nullable / non nullable annotations? -- I think that would probably be ok too. > > > > > > yes, i think so -- [#14519](https://github.com/apache/datafusion/pull/14519) > > [@findepi](https://github.com/findepi) [@alamb](https://github.com/alamb) I Just confirmed that this fixes TPC-DS `66` on Sail! Unfortunately this fix didn't make it into the `45` release. > > > From my perspective it seems like all the tests in DataFusion are passing, so clearly we have some sort of test coverage / gap > > [@shehabgamin](https://github.com/shehabgamin) I wonder if this would be a good time to add more tests that cover your usecase in Sail (or perhaps that would be part of adding Spark functions as discussed in [#5600](https://github.com/apache/datafusion/issues/5600)) 🤔 > > [@alamb](https://github.com/alamb) I agree this would add value. While Spark function test coverage is a starting point, I believe what we're looking for here is broader. I'm not entirely sure about the best approach yet, but I suspect Sail's pattern of catching these bugs/breaks stems from not using DataFusion out-of-the-box. Yes, we have a similar issue downstream in InfluxDB IOx as well (we make extensive use of the DataFusion APIs and so often find bugs that are not covered by existing tests in DataFusion) What we have tried to do is to add the appropriate test coverage for such issues when we find them, though what "appropriate" means varies case by case > > For example, in the TPC-DS `66` case, adding a DataFusion test wouldn't have helped since there was already a test in place. Maybe one approach could be exporting Logical Plans from Sail's tests and using their evaluation as DataFusion test cases? In the case of TPC-DS `66`, we would take the Logical Plan that Sail produced and make the evaluation of that plan a DataFusion test case. That is one approach for sure. If there is a broader pattern of how Sail is creating / using Logical Plans too that we could get tested that would also help a lot. > > P.S. I'm recovering from a harsh cold so sorry if I'm not making much sense lol -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
