logan-keede commented on code in PR #16071: URL: https://github.com/apache/datafusion/pull/16071#discussion_r2094229214
########## datafusion/sqllogictest/test_files/fixed_size_list.slt: ########## Review Comment: > Very nice that this was shipped so fast, thanks! I did not do anything, Everything was already done! > Really nice to get this tested! maybe it's a bit overkill to add dedicated .slt files though. Note that the core logic of the Min/Max function over lists is already tested in > > https://github.com/apache/datafusion/blob/f9326f0745569390fbcb50b2ebeecb9c910549f0/datafusion/sqllogictest/test_files/aggregate.slt#L6998-L7146 > > IMO it should be enough to add a small set of basic tests at the end of `aggregate.slt` proving that the new `FixedSizeList` and `LargeList` types are supported, and rely on the previous vanilla `List` tests for checking that the core logic works. Otherwise, it will be unclear if new tests to the Min/Max aggregations over lists logic should also be replicated for all list variants. > > WDYT? would also be curious to know the stance of other contributors If you consider that we are actually using a generic logic under the hood then yes, but if we think from the perspective of black-box testing then perhaps it is more prudent to have complete test suite. (edit:- or we can just leave a note that `logic is same, and developer/reviewer should account for that.`) on a side note, I would like to avoid adding any more test to aggregate.slt. see this #13723 it already gives 132 error on my laptop(configuration issues that can be fixed,not an actual problem) but yeah it is a mess to parse through. -- 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
