[SQL] Clean up LogicalPlanBuilder#doJoin #34048
Conversation
Currently the local `type` and `condition` variables are unused. After removing them and connected code inside the method, this method seems always to return an exception, so I wonder if it can be removed altogether.
|
Pinging @elastic/es-search-aggs |
|
The parsing is in place in order to tell users using join that it is not supported - without it the grammar would be marked as incorrect which is confusing (since it's not; it's rather that ES SQL doesn't expect JOIN to appear). The type and condition should appear in the error message and it looks like they aren't. The JOIN without USING is parsed since it's an equi-join which, if applied on the same index, is something we aim to support. |
@costin so this is whats missing in this method? Should I open an issue? I can also try fixing if you tell me where these should go in the error message. |
|
I think for the moment it's fine to push this PR as is. Thanks for looking into this. |
|
@costin thanks for the review |
* master: (25 commits) [DOCS] Synchronize location of Breaking Changes (elastic#33588) [DOCS] Synchronizes captialization in top-level titles (elastic#33605) [SQL] Clean up LogicalPlanBuilder#doJoin (elastic#34048) Fix remote cluster seeds fallback (elastic#34090) [ML][HLRC] Replace REST-based ML test cleanup with the ML client (elastic#34109) Handle MatchNoDocsQuery in span query wrappers (elastic#34106) Update MovAvgIT AwaitsFix bug url Bad regex in CORS settings should throw a nicer error (elastic#34035) [HLRC] Support for role mapper expression dsl (elastic#33745) Watcher: Reduce script cache churn by checking for mustache tags (elastic#33978) Fold EngineSearcher into Engine.Searcher (elastic#34082) Mute SpanMultiTermQueryBuilderTests#testToQuery TESTS: Enable DEBUG Logging in Flaky Test (elastic#34091) TEST: Add engine is closed as expected failure msg Adjust bwc version for max_seq_no_of_updates Build DocStats from SegmentInfos in ReadOnlyEngine (elastic#34079) When creating wildcard queries, use MatchNoDocsQuery when the field type doesn't exist. (elastic#34093) [DOCS] Moves graph to docs folder (elastic#33472) Mute MovAvgIT#testHoltWintersNotEnoughData Security: use default scroll keepalive (elastic#33639) ...
Currently the local `type` and `condition` variables are unused and can be removed. This code can be added later again if joins are supported.
Currently the local
typeandconditionvariables are unused. After removingthem and connected code inside the method, this method seems always to return an
exception, so I wonder if it can be removed altogether.