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

Change the default response format to JDBC. #334

Merged
merged 3 commits into from Jan 6, 2020

Conversation

penghuo
Copy link
Contributor

@penghuo penghuo commented Jan 3, 2020

Issue #, if available:
#159

Description of changes:

Problem Statement

Currently, By default the plugin returns original response from Elasticsearch in JSON. There are two major problmes with this format. (1) Not all the SQL query can translated to Elasticsearch DSL directly, so, the post processing logic is required. e.g.,SELECT LOG(MAX(FlightDelayMin)) FROM kibana_sample_data_flights. In this case, the log should be applied on the aggregation result from DSL. (2) For customer, comparing with JDBC format, the original response from Elasticsearch is not easy to understand.

PR includes the following changes

  1. Change the default response format from original Elasticsearch response to JDBC.
  2. Add the setting of the default format if not format params provided. https://github.com/penghuo/sql/blob/jdbc-format/docs/user/admin/settings.rst#opendistro-sql-query-response-format.
  3. Change the Integration Test to used json format which could comptatible with original test cases expection.
  4. Change the document accordingly.
    4.1. https://github.com/penghuo/sql/blob/jdbc-format/docs/user/interfaces/protocol.rst#elasticsearch-dsl
    4.2. https://github.com/penghuo/sql/blob/jdbc-format/docs/user/admin/settings.rst#opendistro-sql-query-response-format
  5. add lombok library.

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

Copy link
Member

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

Just minor comments. Thanks for the changes!


import java.util.Optional;

import static org.junit.Assert.*;
Copy link
Member

Choose a reason for hiding this comment

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

np: wildcard import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* @param requestParams request params.
* @return return true if the response required pretty format, otherwise return false.
*/
public static Boolean isPrettyFormat(Map<String, String> requestParams) {
Copy link
Member

Choose a reason for hiding this comment

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

boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@abbashus abbashus 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 work!

Copy link
Member

@chloe-zh chloe-zh left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

None yet

4 participants