Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL: add "format" for "full" date range queries #48073

Merged
merged 2 commits into from
Oct 22, 2019

Conversation

astefan
Copy link
Contributor

@astefan astefan commented Oct 15, 2019

In case of a conjunction involving the same date field, the optimizer creates a single range query for that field with both from and to populated. In the specific case of date literals generated by ES SQL (NOW(), TODAY(), CURRENT_TIMESTAMP() etc) we are enforcing the date format of the range query so that a potentially custom format for the date field is not used by default by Elasticsearch instead.

Similar logic here

Fixes #48033.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/SQL)

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -821,9 +823,36 @@ protected QueryTranslation asQuery(Range r, boolean onAggs) {
if (onAggs) {
aggFilter = new AggFilter(at.id().toString(), r.asScript());
} else {
Holder<Object> lower = new Holder<>(valueOf(r.lower()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use the holder? Does the closure require a final variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@costin yes.

// no matter the timezone provided by the user
if (format.get() == null) {
DateFormatter formatter = null;
if (lower.get() instanceof ZonedDateTime || upper.get() instanceof ZonedDateTime) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a chance to have different formats between upper and lower? I think so so it might make sense to have two formatters in case of two different formats.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@costin, I tried to test something that has two different formats (like WHERE date0 > CAST(NOW() - INTERVAL 150 YEARS AS DATE) AND date0 < CAST(NOW() AS DATETIME)) but couldn't do it. If you have any ideas I can try to break this, please let me know.

Also, we need to have one format only because that's the one to be used in the range query. Even if the literals themselves will, in I-don't-know-what-scenario, have two different formats, the range query will need only one. In case the range query will end up having two different formats for from and to there will be a parse_exception error from ES side. So, in either case, range will work with one format only for format, from and to fields (all three need to match).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining.

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@matriv matriv added v6.8.5 and removed v7.3.3 labels Oct 16, 2019
@astefan astefan merged commit 020939a into elastic:master Oct 22, 2019
@astefan astefan deleted the 48033_fix branch October 22, 2019 07:13
astefan added a commit that referenced this pull request Oct 22, 2019
astefan added a commit that referenced this pull request Oct 22, 2019
@astefan astefan removed the v6.8.5 label Oct 22, 2019
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 22, 2019
* elastic/master:
  [Docs] Fix opType options in IndexRequest API example. (elastic#48290)
  Simplify Shard Snapshot Upload Code (elastic#48155)
  Mute ClassificationIT tests (elastic#48338)
  Reenable azure repository tests and remove some randomization in http servers  (elastic#48283)
  Use an env var for the classpath of jar hell task (elastic#48240)
  Refactor FIPS BootstrapChecks to simple checks (elastic#47499)
  Add "format" to "range" queries resulted from optimizing a logical AND (elastic#48073)
  [DOCS][Transform] document limitation regarding rolling upgrade with 7.2, 7.3 (elastic#48118)
  Fail with a better error when if there are no ingest nodes (elastic#48272)
  Fix executing enrich policies stats (elastic#48132)
  Use MultiFileTransfer in CCR remote recovery (elastic#44514)
  Make BytesReference an interface (elastic#48171)
  Also validate source index at put enrich policy time. (elastic#48254)
  Add 'javadoc' task to lifecycle check tasks (elastic#48214)
  Remove option to enable direct buffer pooling (elastic#47956)
  [DOCS] Add 'Selecting gateway and seed nodes' section to CCS docs (elastic#48297)
  Add Enrich Origin (elastic#48098)
  fix incorrect comparison (elastic#48208)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL: multi-conditionals on date fields with NOW() don't use the format for range queries
5 participants