somandal commented on PR #9078: URL: https://github.com/apache/pinot/pull/9078#issuecomment-1191938446
> Good catch! I'd suggest doing it slightly different which can handle more general cases: > > 1. In `MinMaxValueBasedSelectionOrderByCombineOperator.MinMaxValueContext`, check if `dataSourceMetadata.isSingleValue()` and put `null` if it is not single valued so that we don't get exception > 2. In `SelectionOrderByOperator.getComparator()`, we have access to the actual metadata of the transformed result, and we may perform the check if we don't want MV to be ordered (currently it skips MV expressions, but IMO it is okay to throw exception to prevent unexpected behavior) Thanks for the suggestions @Jackie-Jiang I didn't understand what you meant by (1) above. What should I set to `null`? For (2), we were thinking about throwing there, but there are a few problems. a) We want to keep the explicit exception throwing backward compatible. Only identifiers go through the `MinMaxValueBasedSelectionOrderByCombineOperator` and already fail during query execution. Transform type expressions won't go through `MinMaxValueBasedSelectionOrderByCombineOperator` and if we now throw if the column is not a singleValue column then if anyone has existing queries with transform they may fail. b) Did you want us to catch multi-value identifiers across all order-by expressions? for the same reason as (a) we wanted to avoid that because people might be running queries today where their first order by expression is single value identifier and with this change their query might fail. c) We also wanted to catch this issue before starting the query execution. That's why we thought planning phase might be a good idea. Let me know your thoughts on the above -- 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]
