ESQL: Drop null columns in text formats#117643
Conversation
elasticsearchmachine
added
needs:triage
astefan
added
:Analytics/ES|QL
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/formatter/TextFormat.java
Outdated
Show resolved
Hide resolved
bpintea
left a comment
bpintea
left a comment
There was a problem hiding this comment.
Looks good, left some small remarks.
| * Format the provided {@linkplain EsqlQueryResponse} optionally including the header lines. | ||
| */ | ||
| public Iterator<CheckedConsumer<Writer, IOException>> format(boolean includeHeader) { | ||
| public Iterator<CheckedConsumer<Writer, IOException>> format(boolean includeHeader, boolean dropNullColumns) { |
There was a problem hiding this comment.
I know that includeHeader was passed here as param, but this object is only created once per response. Maybe it's worth configuring it right from the start by passing these options in the c'tor and building there the [] dropColumns, rather than passing this array down the method chain.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/formatter/TextFormat.java
Outdated
Show resolved
Hide resolved
|
Thank you @bpintea . I have modified the code based on your comments. Please review it when you have time. If there are any other changes needed, please let me know. I am happy to make them. |
|
@elasticsearchmachine test this please |
| return Iterators.concat( | ||
| // if the header is requested return the info | ||
| hasHeader(request) && esqlResponse.columns().isEmpty() == false | ||
| hasHeader(request) && esqlResponse.columns() != null |
There was a problem hiding this comment.
I believe columns will never be null. However, changing the check to see if they are empty might cause some test cases to fail. Perhaps it should be updated to handle empty values and adjust the affected test cases accordingly.
There was a problem hiding this comment.
I didn't try it out, but it looks to me like it should produce the same result. But yes, seems like esqlResponse.columns() will currently never be null.
| case "csv" -> { | ||
| assertEquals(initialValue, "\r\n"); | ||
| assertEquals("\r\n", initialValue); | ||
| initialValue = ""; | ||
| } | ||
| case "tsv" -> { | ||
| assertEquals(initialValue, "\n"); | ||
| assertEquals("\n", initialValue); | ||
| initialValue = ""; | ||
| } |
There was a problem hiding this comment.
This is the test case I mentioned earlier that caused an error. When I first encountered this error, the expected and actual values were reversed, which confused me at first. Therefore, I think it would be better to flip them.
|
@bpintea, I have modified the code to resolve the test failure. However, I can only see this one failure because my IDEA crashes every time when running checks (Maybe I should update my equipment😂). It might be more convenient for external contributors if the elasticsearch-pull-request pipeline were open to the public, similar to the logstash-pull-request-pipeline. |
|
buildkite test this |
bpintea
left a comment
bpintea
left a comment
There was a problem hiding this comment.
LGTM, thanks @kanoshiou!
bpintea
added
the
auto-backport
This PR resolves the issue where, despite setting `drop_null_columns=true`, columns that are entirely null are still returned when using `format=txt`, `format=csv`, or `format=tsv`. Closes elastic#116848
💚 Backport successful
|
This PR resolves the issue where, despite setting `drop_null_columns=true`, columns that are entirely null are still returned when using `format=txt`, `format=csv`, or `format=tsv`. Closes #116848 Co-authored-by: kanoshiou <73424326+kanoshiou@users.noreply.github.com>
A bit late for this PR, but it is from now on. |
|
Thank you very much @bpintea ! |
5 participants
This PR resolves the issue where, despite setting
drop_null_columns=true, columns that are entirely null are still returned when usingformat=txt,format=csv, orformat=tsv.Closes #116848