yy782 commented on PR #65790: URL: https://github.com/apache/doris/pull/65790#issuecomment-5597878422
Thanks for the AI review. I'd like to align on the approach before proceeding. The P1 blocker is that `within` delegates to `rhs->contains(this)`, but `contains` doesn't cover all geometry type pairs — `GeoPoint`/`GeoLine` never override `contains`, and `GeoCircle::contains` only handles Point. Looking at the history, `contains` has been built incrementally (#49665 → #50115). The missing overrides are a known gap from the original design, not an accidental omission. `within`'s delegation is mathematically correct; the issue is that `contains` itself isn't complete yet. Since `ST_Contains` and `ST_Within` are separate features, I'd prefer to: - **Option A (preferred)**: Keep this PR focused on `ST_Within`. Open a separate PR to complete the `contains` type-pair matrix. This PR depends on that one. - **Option B**: Fix `contains` in this PR as well, expanding the scope. Which do you prefer? The test gaps (P2) will be addressed in this PR regardless — I'll add `check_function_all_arg_comb` BE UT and non-constant SQL cases. -- 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]
