[CALCITE-4645] In Elasticsearch adapter, a range predicate should be translated to a range query#4870
Conversation
| if (isSearchWithComplementedPoints(call)) { | ||
| return QueryExpression.create(pair.getKey()).notIn(pair.getValue()); | ||
| } else if (isSearchWithRange(call)) { | ||
| return QueryExpression.create(pair.getKey()).range(pair.getValue()); |
There was a problem hiding this comment.
We already have isSearchWithComplementedPoints to check if it is not in and isSearchWithPoints to check if it is in. Should we can leave range to else?
| * In Elasticsearch adapter, a range predicate should be translated to a range query</a> is | ||
| * fixed. */ | ||
| public static final boolean CALCITE_4645_FIXED = false; | ||
| public static final boolean CALCITE_4645_FIXED = true; |
There was a problem hiding this comment.
I think we can remove it safty.
| .with(AggregationAndSortTest::createConnection) | ||
| .query("select count(*) from view where val1 >= 10 and val1 <=20") | ||
| .returns("EXPR$0=1\n"); | ||
| .returns("EXPR$0=0\n"); |
There was a problem hiding this comment.
This actually is a wrong case.
There was a problem hiding this comment.
test dataset is :
String doc1 = "{cat1:'a', cat2:'g', val1:1, cat4:'2018-01-01', cat5:1}";
String doc2 = "{cat2:'g', cat3:'y', val2:5, cat4:'2019-12-12', cat6:'text1'}";
String doc3 = "{cat1:'b', cat2:'h', cat3:'z', cat5:2, val1:7, val2:42}";
val1 >= 10 and val1 <=20 , doc1/2/3 can not fit the condition.
So it is right in my view.
| return this; | ||
| } | ||
|
|
||
| private RangeQueryBuilder createRangeQuery(Range<?> range, LiteralExpression literal) { |
There was a problem hiding this comment.
Can all data types support rewrite to "range query"? Perhaps we should add some cases for different data types
There was a problem hiding this comment.
Yes, I added some new test cases, and the results met expectations.
|
To be honest, Calcite's Elasticsearch adapter code is riddled with issues. I previously maintained an internal production-ready version of it, which amounted to practically a complete refactor. |
You may very well be right, but even a small improvement is better than no improvement at all. Perhaps we could also discuss refactoring this section; it seems the community encourages doing so. |
|
I will review this over the next couple of days. |
| } | ||
| } | ||
|
|
||
| return addFormatIfNecessary(literal, rangeQuery); |
There was a problem hiding this comment.
In createRangeQuery, directly check if the range endpoint value is GregorianCalendar to determine whether to add a format, instead of relying on literal.value().
There was a problem hiding this comment.
For SEARCH/Sarg-type literals, literal.value() follows the isSarg() → sargValue() branch, returning a List<Object>; it will never return a GregorianCalendar. This implies that range queries on date fields will not set the format("date_time") attribute.
There was a problem hiding this comment.
OK, modified as your suggestion.
| @@ -215,7 +216,8 @@ private static boolean supportedRexCall(RexCall call) { | |||
| * @return true if it isSearchWithPoints or isSearchWithComplementedPoints, other false | |||
| */ | |||
| static boolean canBeTranslatedToTermsQuery(RexCall search) { | |||
There was a problem hiding this comment.
The method name may no longer be accurate.
There was a problem hiding this comment.
The method name here is not relevant to this modification; it may be modified separately in a more suitable Jira implementation in the future.
| @@ -215,7 +216,8 @@ private static boolean supportedRexCall(RexCall call) { | |||
| * @return true if it isSearchWithPoints or isSearchWithComplementedPoints, other false | |||
| */ | |||
| static boolean canBeTranslatedToTermsQuery(RexCall search) { | |||
There was a problem hiding this comment.
It looks like JavaDoc is a bit outdated.
…translated to a range query
xuzifu666
added
the
LGTM-will-merge-soon
|
If no other comments, I will merge this pr in 24H. |
|
3 participants
jira：https://issues.apache.org/jira/browse/CALCITE-4645