lucasfang commented on PR #301: URL: https://github.com/apache/paimon-cpp/pull/301#issuecomment-5612055259
Thanks, the selection logic itself is correct and the test matrix is thorough. Both earlier review comments are addressed at this head: the TIMESTAMP/DECIMAL128 eager arms are gone and the benchmark moved to `benchmark/`. My concerns are about the performance model rather than correctness, and most of them come from the interaction with #306. ## The three commits have very different value `b9061c0` (`num_fields()`) is independent, tiny and obviously correct. Please split it out with `TestLeafFieldBoundsOnWideSlicedBatch` and the integration test into its own PR so it can merge on its own; there is no reason to couple it to the selection work. `4785b98` (typed equality) is a strict subset of #306 and should be dropped, see below. The selection commits are correct but their payoff shrinks a lot once #306 lands, and the benchmark as posted does not isolate the cost that decides whether they are net positive. -- 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]
