gabotechs commented on code in PR #16071: URL: https://github.com/apache/datafusion/pull/16071#discussion_r2094196073
########## datafusion/sqllogictest/test_files/fixed_size_list.slt: ########## Review Comment: Really nice to get this tested! maybe it's a bit overkill to add dedicated .slt files just for this 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? -- 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
