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.

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

Move result-to-array logic from SQL layer into QueryToolChests. #9130

Merged
merged 4 commits into from Jan 16, 2020

Conversation

gianm
Copy link
Contributor

@gianm gianm commented Jan 5, 2020

The main changes are the removal of query-result-to-array code from org.apache.druid.sql.calcite.rel.QueryMaker, and the addition of these two methods to QueryToolChest:

List<String> resultArrayFields(QueryType query);
Sequence<Object[]> resultsAsArrays(QueryType query, Sequence<ResultType> resultSequence);

I think this improves the design of the code a little bit, but the main benefit is really to support joins on subqueries. The change is needed by this item from the proposal #8728:

Allow joining on to "query" datasources as well. To make this work, we’ll need to add a sense of a ‘standard translation’ of results from certain query types into flat schemas that we can offer column selectors on top of. There may be more than one way to do this, since certain query types (notably, topN and scan) return nested results in some cases. We could do this by adding a new QueryToolChest method.

@@ -269,4 +270,50 @@ public ObjectMapper decorateObjectMapper(final ObjectMapper objectMapper, final
{
return segments;
}

/**
* Returns a list of field names in the order than {@link #resultsAsArrays} would return them. The returned list will
Copy link
Member

Choose a reason for hiding this comment

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

nit: should this be 'Returns a list of field names in the order that ...'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should. I updated it. Thanks.

Copy link
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

🤘

@gianm gianm merged commit bd49ec0 into apache:master Jan 16, 2020
@gianm gianm deleted the result-to-array branch January 16, 2020 23:42
@jihoonson jihoonson added this to the 0.18.0 milestone Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants