Skip to content

Implement interfaces for RowExpressionTranslation#13491

Merged
highker merged 1 commit intoprestodb:masterfrom
sachdevs:example-framework
Oct 15, 2019
Merged

Implement interfaces for RowExpressionTranslation#13491
highker merged 1 commit intoprestodb:masterfrom
sachdevs:example-framework

Conversation

@sachdevs
Copy link
Contributor

@sachdevs sachdevs commented Oct 2, 2019 •
edited
Loading

Implement Row Expression translation interfaces in new module presto-expression.

NOTE: See an example implementation using these interfaces in #13526

== NO RELEASE NOTE ==

@sachdevs sachdevs mentioned this pull request Oct 2, 2019
Closed
@sachdevs sachdevs requested review from hellium01 and highker October 2, 2019 03:47
highker
highker reviewed Oct 2, 2019
View reviewed changes
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

High-level comments only

@hellium01
Copy link
Contributor

hellium01 commented Oct 2, 2019

@sachdevs , let's sync up offline? I made some changes on top @highker and your code, which I hope can make discussing it a little easier?
hellium01@fa01300

@sachdevs
Copy link
Contributor Author

sachdevs commented Oct 2, 2019

Added Yi's changes and now addressing James' comments + cleaning up further.

@sachdevs
Copy link
Contributor Author

sachdevs commented Oct 3, 2019 •
edited
Loading

Added tests, ensured basic code paths work correctly, and cleaned up further.
Functionality TODOs:

  • Add optimizer flag to Jdbc (for turning on/off feature)
  • Add JdbcOptimizerProvider to executed code path so that the optimizer can be used if desired
  • Add other Jdbc translation functions? @hellium01 how far do we want to go for the JDBC example?

Other TODOs:

  • Split this into multiple commits
  • Add integration tests for end-to-end verification
  • Add more javadocs for SPI, clean up further

@sachdevs
Copy link
Contributor Author

sachdevs commented Oct 4, 2019

Added another test suite, now splitting up commits + cleaning up.

@sachdevs sachdevs force-pushed the example-framework branch 2 times, most recently from afed528 to 848313c Compare October 7, 2019 18:36
@sachdevs sachdevs requested a review from highker October 7, 2019 19:44
@sachdevs
Copy link
Contributor Author

sachdevs commented Oct 7, 2019

Feel free to take a look for any high-level mistakes/ things I'm still missing before I start to get deeper into implementing the JDBC connector using this interface.

highker
highker reviewed Oct 8, 2019
View reviewed changes
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

Discussed offline with some changes based on highker@cb77e79

@rongrong
Copy link
Contributor

rongrong commented Oct 8, 2019

How confident are we about this is the right approach to do translation and these are the right interfaces? Should we consider putting these in a separate package rather than add all them to spi until we get better understanding and confidence that these are the right things to do?

@sachdevs
Copy link
Contributor Author

sachdevs commented Oct 8, 2019

After discussing with James, moving to a separate module and decoupling SPI from this PR.

@sachdevs sachdevs mentioned this pull request Oct 9, 2019
Merged
@sachdevs
Copy link
Contributor Author

sachdevs commented Oct 9, 2019

Refactored to be in separate new top-level library module presto-expressions. Moved Jdbc implementation commit to separate PR. Let me know if I was correct in assuming ConnectorPlanOptimizerManager goes into the new module as well.

@sachdevs sachdevs requested a review from highker October 9, 2019 20:57
highker
highker reviewed Oct 9, 2019
View reviewed changes
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

Could you add unit tests to cover the critical codepath?

@sachdevs
Copy link
Contributor Author

sachdevs commented Oct 10, 2019

Cleaned up, adding unit tests now.

@sachdevs sachdevs force-pushed the example-framework branch 2 times, most recently from 18bb400 to 8bfaf9d Compare October 11, 2019 01:45
@sachdevs
Copy link
Contributor Author

sachdevs commented Oct 11, 2019

Finished writing tests. Let me know if we would like more coverage on any specific code path. Currently I'm hitting most parts of the translator and annotation parser.

@sachdevs sachdevs requested a review from highker October 11, 2019 01:46
highker
highker reviewed Oct 11, 2019
View reviewed changes
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

Some comments

@highker highker changed the title [WIP] Implement interfaces for RowExpressionTranslation Implement interfaces for RowExpressionTranslation Oct 11, 2019
@sachdevs sachdevs requested a review from highker October 11, 2019 21:06
@sachdevs
Copy link
Contributor Author

sachdevs commented Oct 11, 2019 •
edited
Loading

Looks like I missed the checkstyle run on my last revision, fixed those issues now.

The refactor also resulted in me getting rid of the tests that check the "automatic" subtree translation by the library. Let me know if there are any other tests I am missing. Is it worthwhile to unit test annotation parsing/FunctionTranslator in isolation? I.e. make a temp class to parse and ensure correct entries show up in the functionMapping?

highker
highker approved these changes Oct 12, 2019
View reviewed changes
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

LGTM % minor comments

hellium01
hellium01 reviewed Oct 12, 2019
View reviewed changes
Copy link
Contributor

@hellium01 hellium01 left a comment

Choose a reason for hiding this comment

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

Looks good, have some comments but we can iterate on later commits.

...ns/src/main/java/com/facebook/presto/expressions/translator/RowExpressionTreeTranslator.java Outdated
Copy link
Contributor

@hellium01 hellium01 Oct 12, 2019

Choose a reason for hiding this comment

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

We don't need to put this logic in the RowExpressionTreeTranslator, the user of translateWith need to do before they call translateWith.

Copy link
Contributor Author

@sachdevs sachdevs Oct 14, 2019

Choose a reason for hiding this comment

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

Are there any side-effects of this assumption? I.e. if the user does not convert the expression/predicate to conjunctive form, could their be any resultant issues other than building the minimal converted rowexpression? I just want to make sure that by moving this out we dont make the code less fool-proof.

Copy link

@highker highker Oct 14, 2019

Choose a reason for hiding this comment

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

I don't think we need logicalRowExpressions. That is too specialized for predicates.

Copy link
Contributor

@hellium01 hellium01 Oct 14, 2019

Choose a reason for hiding this comment

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

I don't think this convertToConjunctiveNormalForm is needed for all translations, it is only used to make predicate pushdown easier, so definitely remove it from the common interface.

...ns/src/test/java/com/facebook/presto/expressions/translator/TestRowExpressionTranslator.java Outdated
Copy link
Contributor

@hellium01 hellium01 Oct 12, 2019

Choose a reason for hiding this comment

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

We also might want to cover:
some types of constant that is not translatable.

Copy link
Contributor Author

@sachdevs sachdevs Oct 14, 2019

Choose a reason for hiding this comment

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

what type of constant would that be? untranslatable constants wouldnt make it past expression creation no? since the type provider entry will be missing?

...ons/src/main/java/com/facebook/presto/expressions/translator/TranslatorAnnotationParser.java Outdated
Copy link
Contributor

@hellium01 hellium01 Oct 12, 2019

Choose a reason for hiding this comment

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

I would prefer we do some checking on return type and argument type during parseFunctionDefinition rather than throw later when we call translation if we have wrongly defined translate functions.

Copy link
Contributor Author

@sachdevs sachdevs Oct 14, 2019

Choose a reason for hiding this comment

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

Yeah I can see why that is useful. Makes the code more defensible and we fail earlier.

@sachdevs
Copy link
Contributor Author

sachdevs commented Oct 14, 2019

Updated PR with comments from @highker and @hellium01. Just looking for verification on what we need to do about the unresolved comments and lets finish this up 😄

@sachdevs sachdevs requested a review from hellium01 October 14, 2019 19:00
hellium01
hellium01 approved these changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@hellium01 hellium01 left a comment

Choose a reason for hiding this comment

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

Please definitely remove the deterministicEvaluator/functionResolution/functionMetadataManger from translateWith, otherwise, it looks good.

...ns/src/main/java/com/facebook/presto/expressions/translator/RowExpressionTreeTranslator.java Outdated
Copy link
Contributor

@hellium01 hellium01 Oct 14, 2019

Choose a reason for hiding this comment

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

I don't think this convertToConjunctiveNormalForm is needed for all translations, it is only used to make predicate pushdown easier, so definitely remove it from the common interface.

@sachdevs
Copy link
Contributor Author

sachdevs commented Oct 14, 2019 •
edited
Loading

Done, removed those params. 🚢

highker
highker reviewed Oct 14, 2019
View reviewed changes
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

some comments

...ns/src/main/java/com/facebook/presto/expressions/translator/RowExpressionTreeTranslator.java Outdated
Copy link

@highker highker Oct 14, 2019

Choose a reason for hiding this comment

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

I don't think we need logicalRowExpressions. That is too specialized for predicates.

@highker highker self-assigned this Oct 15, 2019
@highker highker merged commit 2bd86f8 into prestodb:master Oct 15, 2019
@wypb wypb mentioned this pull request Jul 9, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@highker highker highker approved these changes

@hellium01 hellium01 hellium01 approved these changes

Assignees

@highker highker

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sachdevs @hellium01 @rongrong @highker @facebook-github-bot