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

Remove of null column in JDBC output when script function is present #116

Merged
merged 3 commits into from
Jul 16, 2019
Merged

Remove of null column in JDBC output when script function is present #116

merged 3 commits into from
Jul 16, 2019

Conversation

galkk
Copy link
Contributor

@galkk galkk commented Jul 16, 2019

Issue #, if available: 111

Description of changes:

Fix of JDBC output when there's script function in group by. It is done by additional analysis in schema preparation step to differentiate script and method fields.

After this fix the output

POST _opendistro/_sql?format=jdbc
{
  "query": "SELECT date_format(utc_time, 'YYYY-MM'),  count(*) from kibana_sample_data_logs group by date_format(utc_time, 'YYYY-MM')"
}

returns, as expected

{
  "schema": [
    {
      "name": "date_format_1721549462",
      "type": "text"
    },
    {
      "name": "COUNT(*)",
      "type": "long"
    }
  ],
  "total": 3,
  "datarows": [
    [
      "2018-08",
      7132
    ],
    [
      "2018-09",
      4552
    ],
    [
      "2018-07",
      2321
    ]
  ],
  "size": 3,
  "status": 200
}

Testing:
integration test, manual via Kibana UI

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

@galkk galkk requested review from dai-chen and abbashus July 16, 2019 18:22
@@ -35,8 +36,9 @@
private String alias;
private NestedType nested;
private ChildrenType children;
private SQLExpr expression;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the use of this new field member. Did I miss it anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see answer below. It's part of upcoming PR that I've slipped here. I can move it into different PR or leave it as is, because the change will be part of next PR anyway

List<Field> groupByFields = select.getGroupBys().isEmpty() ? new ArrayList<>() : select.getGroupBys().get(0);

for (Field selectField : select.getFields()) {
if (selectField instanceof MethodField && !selectField.isScriptField()) {
Copy link
Member

@dai-chen dai-chen Jul 16, 2019

Choose a reason for hiding this comment

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

I assume what we want exactly here is de-duplicate. The !selectField.isScriptField check works on the assumption that there is no aggregate function implemented by painless script right? If so, is this true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's even worse (because as per Elastic's DSL script aggregations can be for both Bucket and Metric aggregations), but it will require considerable redesign, because the process of transforming of parsed SQL into current domain model loses a lot of information. For example - for query like

select date_format(format), count(*)
from index
group by date_format(format)
order by date_format(format)

there's no easy way to understand that the date_format is actually the same, because at this stage we're operating only with transformed expressions.

Adding of SQLExpr into field information in this PR is related to future attempt to de-duplication that is necessary for order by.

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.

Looks OK to me, just address @dai-chen's concern.

@galkk galkk merged commit 09134c9 into opendistro-for-elasticsearch:master Jul 16, 2019
@galkk galkk deleted the fix-of-unnecessary-column-in-jdbc branch July 16, 2019 20:16
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.

3 participants