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.

Sign up for GitHub

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

Jump to bottom

SQL: Return Intervals in SQL format for CLI #37602

Merged
merged 4 commits into from
Jan 22, 2019
Merged

SQL: Return Intervals in SQL format for CLI #37602

merged 4 commits into from
Jan 22, 2019

Conversation

astefan
Copy link
Contributor

@astefan astefan commented Jan 18, 2019 •
edited
Loading

This PR adds a new CLI mode and removes the CLI client_id. Uses the new Mode to decide what format to use for Intervals in the responses to CLI requests. Fixes #36432.

The PR also fixes #29970 by not making any conversions, since the received data is already formatted.

Another small improvement in this PR is the use of the right Mode for cursor close requests.

Also, a nice consequence of these changes is that the CLI will not greatly depend on the future SQL protocol changes, but it will only display what receives from the server, paving the way for safe changes like #36186.

@astefan
Add separate CLI Mode
0a3f949 
* Use the correct Mode for cursor close requests
* Updated tests and cleaned up CliFormatter
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@astefan astefan requested review from costin and matriv January 18, 2019 09:00
matriv
matriv approved these changes Jan 18, 2019
View reviewed changes
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. Left a comment

...plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java
@@ -173,7 +175,12 @@ public static XContentBuilder value(XContentBuilder builder, Mode mode, Object v
ZonedDateTime zdt = (ZonedDateTime) value;
// use the ISO format
builder.value(StringUtils.toString(zdt));
} else {
} else if (mode == CLI && value != null && value.getClass().getSuperclass().getSimpleName().equals(INTERVAL_CLASS_NAME)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to use the isAssignableFrom() and avoid the string check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, but Interval is not visible in the action project.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, thx!

@astefan
Copy link
Contributor Author

astefan commented Jan 18, 2019

run the gradle build tests 1

@astefan
Copy link
Contributor Author

astefan commented Jan 18, 2019

run the gradle build tests 2

@astefan
Copy link
Contributor Author

astefan commented Jan 18, 2019

:-(( run the gradle build tests 1

@astefan
Copy link
Contributor Author

astefan commented Jan 18, 2019

run the gradle build tests 1

1 similar comment
@astefan
Copy link
Contributor Author

astefan commented Jan 18, 2019

run the gradle build tests 1

@astefan
Copy link
Contributor Author

astefan commented Jan 18, 2019

run the gradle build tests 1

3 similar comments
@astefan
Copy link
Contributor Author

astefan commented Jan 18, 2019

run the gradle build tests 1

@astefan
Copy link
Contributor Author

astefan commented Jan 18, 2019

run the gradle build tests 1

@costin
Copy link
Member

costin commented Jan 19, 2019

run the gradle build tests 1

costin
costin approved these changes Jan 19, 2019
View reviewed changes
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. The mode has changed but I didn't see any update on the metrics side - is it counted for REST?

@astefan
Copy link
Contributor Author

astefan commented Jan 22, 2019

@costin the metrics are built from the list of QueryMetrics values and CLI was already there. And it's a String comparison, so from this point of view it shouldn't be any difference.

@costin @matriv I've pushed another commit which is changing how the CliFormatter is named and behaves. The CLI and the TEXT format (as in ?format=txt) both use the CliFormatter but the TEXT format needs a different formatting behavior than the CLI one (the CLI receives the data already formatted while the text format needs org.elasticsearch.xpack.sql.proto.StringUtils), thus the change in the CliFormatter. Given that it's used for both CLI and TEXT format, I've changed its name.
Can you, please, have another look?

costin
costin approved these changes Jan 22, 2019
View reviewed changes
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.

LGTM2

matriv
matriv approved these changes Jan 22, 2019
View reviewed changes
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!

@astefan astefan merged commit 7507af2 into elastic:master Jan 22, 2019
@astefan astefan deleted the 36432fix branch January 22, 2019 12:55
astefan added a commit that referenced this pull request Jan 22, 2019
@astefan
SQL: Return Intervals in SQL format for CLI (#37602)
e80c5f3 
* Add separate CLI Mode
* Use the correct Mode for cursor close requests
* Renamed CliFormatter and have different formatting behavior for CLI and "text" format.

(cherry picked from commit 7507af2)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 22, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into remove-warn-date
1e9d224 
* elastic/master: (43 commits)
  Remove remaining occurances of "include_type_name=true" in docs (elastic#37646)
  SQL: Return Intervals in SQL format for CLI (elastic#37602)
  Publish to masters first (elastic#37673)
  Un-assign persistent tasks as nodes exit the cluster (elastic#37656)
  Fail start of non-data node if node has data (elastic#37347)
  Use cancel instead of timeout for aborting publications (elastic#37670)
  Follow stats api should return a 404 when requesting stats for a non existing index (elastic#37220)
  Remove deprecated FieldNamesFieldMapper.Builder#index (elastic#37305)
  Document that date math is locale independent
  Bootstrap a Zen2 cluster once quorum is discovered (elastic#37463)
  Upgrade to lucene-8.0.0-snapshot-83f9835. (elastic#37668)
  Mute failing test
  Fix java time formatters that round up (elastic#37604)
  Removes awaits fix as the fix is in. (elastic#37676)
  Mute failing test
  Ensure that max seq # is equal to the global checkpoint when creating ReadOnlyEngines (elastic#37426)
  Mute failing discovery disruption tests
  Add note about how the body is referenced (elastic#33935)
  Define constants for REST requests endpoints in tests (elastic#37610)
  Make prepare engine step of recovery source non-blocking (elastic#37573)
  ...
@astefan astefan mentioned this pull request Jan 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matriv matriv matriv approved these changes

@costin costin costin approved these changes

Assignees
No one assigned
Labels
:Analytics/SQL SQL querying >bug v6.7.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
5 participants
@astefan @elasticmachine @costin @matriv @colings86