Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Fix for ExprValueFactory construct issue #898

Merged

Conversation

rupal-bq
Copy link
Contributor

@rupal-bq rupal-bq commented Dec 8, 2020

Issue #858, #867

Description of changes:

  • added date, time, and datetime in ExprValueFactory
  • added unit tests
  • added manual integration tests (SQLite returned the wrong type in a schema and the comparison test failed).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #898 (103862a) into develop (793d92d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #898   +/-   ##
==========================================
  Coverage      99.85%   99.85%           
- Complexity      2148     2153    +5     
==========================================
  Files            216      216           
  Lines           4845     4856   +11     
  Branches         318      323    +5     
==========================================
+ Hits            4838     4849   +11     
  Misses             5        5           
  Partials           2        2           
Impacted Files Coverage Δ Complexity Δ
...arch/data/value/ElasticsearchExprValueFactory.java 100.00% <100.00%> (ø) 60.00 <0.00> (+5.00)
...icsearch/sql/sql/parser/AstAggregationBuilder.java 100.00% <0.00%> (ø) 15.00% <0.00%> (ø%)
...rch/sql/sql/parser/context/QuerySpecification.java 100.00% <0.00%> (ø) 15.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 793d92d...103862a. Read the comment docs.

@dai-chen
Copy link
Member

dai-chen commented Dec 8, 2020

Is #867 same issue?

@rupal-bq
Copy link
Contributor Author

rupal-bq commented Dec 8, 2020

Is #867 same issue?

Thanks for noticing, it's the same issue. I will add a test for that case as well.

Copy link
Contributor

@penghuo penghuo left a comment

Choose a reason for hiding this comment

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

Thanks for the change.

@penghuo penghuo merged commit 0cda05c into opendistro-for-elasticsearch:develop Dec 8, 2020
joshuali925 pushed a commit that referenced this pull request Dec 9, 2020
* add date & time

* add datetime

* try comparison test

* add manual IT

* add integration test for issue #867

Co-authored-by: Rupal Mahajan <>
penghuo pushed a commit that referenced this pull request Dec 15, 2020
* add date & time

* add datetime

* try comparison test

* add manual IT

* add integration test for issue #867

Co-authored-by: Rupal Mahajan <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants