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

feat: Implement complex expressions for table functions #3683

Merged
merged 3 commits into from
Oct 29, 2019

Conversation

purplefox
Copy link
Contributor

@purplefox purplefox commented Oct 26, 2019

Description

Fixes #3514

This is the 2nd PR in a stack. Please note this PR is stacked on #3685, so please don't review commits from the previous PR again.

With this PR table functions can now be arbitrarily complex expressions.

Testing done

New QTT test

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

throw new IllegalArgumentException("Can't find input column " + columnName);
}
final Expression expression = functionCall.getArguments().get(0);
final ExpressionMetadata expressionMetadata =
Copy link
Contributor

@agavra agavra Oct 28, 2019

Choose a reason for hiding this comment

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

I feel like this doesn't belong here - instead, we should be resolving all complex expressions before running the KudtfFlatMapper. This has a few benefits, the first is the performance benefit that I had mentioned earlier (we want to evaluate complex expressions once, not per every output row) and the second is that we're not treating arguments to UDTFs any differently than any other expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow - this code is before the FlatMapper is run.

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked offline, because this PR is blocking lots of others I'm going to let it through as is and come up with a more detailed ticket describing what I mean by the comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

@agavra agavra requested a review from a team October 28, 2019 22:57
@purplefox purplefox force-pushed the complex_select_1 branch 2 times, most recently from 3852c7e to 3f9fe9a Compare October 28, 2019 23:53
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

LGTM with the caveat that we'll look into my suggestion after these features are merged

@purplefox purplefox merged commit 200022b into confluentinc:master Oct 29, 2019
@purplefox purplefox deleted the complex_select_1 branch January 26, 2020 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Evaluate table functions against more complex expressions
2 participants