Skip to content

Add PredicateCompiler to SPI#12729

Merged
mbasmanova merged 4 commits into
prestodb:masterfrom
mbasmanova:predicate-compiler
May 15, 2019
Merged

Add PredicateCompiler to SPI#12729
mbasmanova merged 4 commits into
prestodb:masterfrom
mbasmanova:predicate-compiler

Conversation

@mbasmanova
Copy link
Copy Markdown
Contributor

No description provided.

@mbasmanova mbasmanova added the aria Presto Aria performance improvements label Apr 26, 2019
@mbasmanova mbasmanova force-pushed the predicate-compiler branch 3 times, most recently from 49203be to b781698 Compare April 26, 2019 19:12
@mbasmanova mbasmanova changed the title [WIP] Add PredicateCompiler to SPI Add PredicateCompiler to SPI Apr 26, 2019
Copy link
Copy Markdown

@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 + nits only. Will dig into logic details soon. I think this service can be generalized into arbitrary RowExpression compiler service. That will be a very useful tool in SPI. Other than that, it's in good shape.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit

int[] inputChannels = result.getInputChannels()
                .getInputChannels()
                .stream()
                .mapToInt(Integer::intValue)
                .toArray();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've been following the convention to put a line break after stream() and then put each transformation on a separate line.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

compileFilterInternal(RowExpression predicate, Optional<String> classNameSuffix)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

High-level comments on this class: There are many overlappings of this class with PageFunctionCompiler. But of course, there are some subtleties are different. Not sure if it worth extracting the common code path into utils. If not, can we add some basic tests like TestPageFunctionCompiler?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We might need to switch all filters to predicates in this file.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we also rename com.facebook.presto.sql.planner.DeterminismEvaluator into ExpressionDeterminismEvaluator and mark it deprecated? That class is a complete hack. It is also incorrect to some extend. It should go away in the future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I might have missed something but RowExpressionCompiler is a quite powerful tool to compile any RowExpression beyond predicates. So, in theory, result could be Object.class. Is it possible to make this class a generic compiler rather than only for predicates?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@highker It should be possible to change this into a generic expression evaluator, but what's the use case? If a generic version returns an Object, then current use case for the filter would require an undesirable cast to boolean.

@wenleix wenleix self-requested a review April 29, 2019 19:16
@mbasmanova mbasmanova force-pushed the predicate-compiler branch 5 times, most recently from 38f798b to aef610c Compare May 13, 2019 17:46
@mbasmanova
Copy link
Copy Markdown
Contributor Author

@highker James, thank you for review. I addressed your comments and updated the PR. Would you take another look?

@highker highker self-requested a review May 13, 2019 20:43
Copy link
Copy Markdown

@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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As discussed offline, let's put a javadoc to denote a few things:

  • this is an experimental API
  • the goal is do to filter reordering in connector to workaround lazy block loading everything
  • we should explore the possibility having a new interface in lazy block (or maybe new block implementation) to selectively load blocks

cc: @arhimondr @wenleix @tdcmeehan

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@highker Added javadoc to clarify that this is an experimental API.

/**
 * @apiNote An experimental API to allow Hive connector to implement smart
 * filtering on the encoded data to avoid materializing data unnecessarily.
 */
public interface PredicateCompiler
{

@mbasmanova mbasmanova force-pushed the predicate-compiler branch from aef610c to d7294bb Compare May 14, 2019 19:15
@mbasmanova mbasmanova merged commit b430a56 into prestodb:master May 15, 2019
@mbasmanova mbasmanova deleted the predicate-compiler branch May 24, 2019 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aria Presto Aria performance improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants