-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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: introduce the columnar option for REST requests #39287
Conversation
modes) for json, yaml, smile and cbor formats.
Pinging @elastic/es-search |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! Can we have a test that "columnar" is not allowed for translate requests?
|
||
private String columnarParameter(boolean columnar) { | ||
String columnarParameter = ",\"columnar\":" + columnar; | ||
if (columnar == false && randomBoolean()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's more clear if you use the if in the beginning of the method and return ""
, otherwise return the string
without the need of the columnarParameter
variable.
} | ||
|
||
private String cursor = ""; | ||
/* | ||
* Using the Boolean object here so that SqlTranslateRequest to set this to null (since it doesn't need a "columnar" parameter). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe better: since "columnar" parameter is optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite. columnar
may be optional for a sql query, but it doesn't have a place for a translate query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, sorry, I mixed it up.
if (columnar) { | ||
// columns can be specified (for the first REST request for example), or not (on a paginated/cursor based request) | ||
// if the columns are missing, we take the first rows' size as the number of columns | ||
long columnsCount = columns != null ? columns.size() : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imho this is superfluous and can be removed.
If the rows().get(0).size() != columns.size()
(when columns are there -> 1st request) then maybe we should throw an exception as this is a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This can be merged only after adding versioning to our protocols...
individual record/document represents one line/row. For certain formats, {es-sql} can return the results | ||
in a columnar fashion: one row represents all the values of a certain column from the current page of results. | ||
|
||
A columnar style of results makes sense for certain formats, thus it's available for `json`, `yaml`, `cbor` and `smile`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid the "for certain formats" repetition, this could be rephrased to "The following formats can be returned in columnar orientation".
-------------------------------------------------- | ||
// TESTRESPONSE[s/sDXF1ZXJ5QW5kRmV0Y2gBAAAAAAAAAAEWWWdrRlVfSS1TbDYtcW9lc1FJNmlYdw==:BAFmBmF1dGhvcgFmBG5hbWUBZgpwYWdlX2NvdW50AWYMcmVsZWFzZV9kYXRl\+v\/\/\/w8=/$body.cursor/] | ||
|
||
Any subsequent calls using a `cursor` still have to contain the `columnar` parameter if it is wanted the results to be returned this way, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"if it is wanted the results to be ..." - "still have... to preserve the orientation "
/** | ||
* Should format the values in a columnar fashion or not (default false). | ||
* Depending on the format used (csv, tsv, txt, json etc) this setting will be taken into | ||
* consideration or not, depending on if it even makes sense for that specific format or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"depending on _whether" it makes .."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add "columnar" option for REST requests (but be lenient for non-"plain" modes) for json, yaml, smile and cbor formats. * Updated documentation (cherry picked from commit 5b7e0de)
Implements elastic/elasticsearch#39287 (cherry picked from commit 39ad9a2)
This PR introduces the
columnar
option which, forjson
,yaml
,cbor
andsmile
format, will return the results in a columnar fashion: all the values of a certain column from the current page of results will be placed on a single line.This option is valid for REST requests (no JDBC or CLI).
Addresses #37702.