mikebridge commented on PR #45912: URL: https://github.com/apache/superset/pull/45912#issuecomment-6635174306
Thanks for the careful rounds on this, @aminghadersohi. Each one found a fixture shape where the checker either let a misbehaving provider through or failed a correct one, and that made something clear: proving NULL placement from a provider's rows alone doesn't converge. So we're narrowing the change instead of adding more checks: - The SDK will say NULL placement, and the order of tied rows, are provider-defined, as before. - The original problem was in Superset's own result cache: when it re-aggregates a cached result, it re-sorts in pandas (NULLs last) and then applies the limit, so it can keep different rows than a NULLs-first engine would. The cache will now only answer a limited query that way when the whole re-aggregated result fits within the limit; otherwise it asks the provider. That covers NULLs, ties and string ordering at once. - The NULL-placement checker in `testing.py` and its fixtures go away. I'll push the reduced version here (or open a small replacement PR and link it) and re-request your review. Your last two suggestions only touch the checker, so they won't be needed. -- 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]
