Remove predicate from Constraint#29289
Conversation
5e0db2f to
7a942c8
Compare
71ea116 to
02fd4ff
Compare
0b5d5ca to
adf1267
Compare
| } | ||
| } | ||
| @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "@type") | ||
| public interface PartitionExpression {} |
There was a problem hiding this comment.
Partitions are an internal connector detail. We've intentionally avoided exposing such a concept to the engine.
There was a problem hiding this comment.
This class isn't meant to carry connector details back to the engine, it's designed to represent a filter expression that the connector can't recognize as a ConnectorExpression, but which we still want to push down for evaluation. As I understand it, it's used mostly for partition pruning in a specific scenario though, I renamed it to ConstraintExpression to avoid confusion, please suggest the name.
We need the main module's ability to leverage the IR optimizer for evaluating values that come from the connector. Previously we used a predicate to check ,from the connector's perspective, they are exposing exactly the same data to the engine (unless I'm misunderstanding something).
For the newly added PartitionExpression (now ConstraintExpression), I've also been thinking we could carry this info into a table handle. But in any case, I think we should carry the context as an actual Expression, though it's invisible to connectors, that means we need an abstract ConstraintExpression interface for carrying this. What do you think?
3160ebc to
6a60994
Compare
c02645d to
4e669b3
Compare
|
@martint Please take a look when you are available, thanks |
4e669b3 to
1a1d387
Compare
1a1d387 to
e7d6e28
Compare
e7d6e28 to
70e9084
Compare
`useState<boolean>(thread.resolved === true)` only ran at mount, so a thread that arrived with `resolved == null` on the initial REST fetch stayed open even after the follow-up GraphQL fetch flipped it to true. On long PRs (e.g. trinodb/trino#29289) this left a wall of resolved threads expanded by default. Derive `folded` from props each render, with a `foldOverride` that pins the user's manual chevron click so subsequent refreshes don't re-fold a thread they explicitly opened. Same fix in both surfaces — the main PR detail page (ReviewThreadCard) and the inline-in-diff view (DiffViewerScreen). The optimistic-resolve test now asserts via the always-visible resolved pill instead of the Unresolve button — the button now auto-folds away on click (matching github.com), but the optimistic local-state patch is still observable in the header pill. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The direction is right — removing the non-serializable Alternative: synthetic Instead of a new opaque The serialized IR expression is the same thing Add a general Key consequence: With a general evaluator, Why this is preferable to the opaque approach
The |
70e9084 to
738ba05
Compare
8fea433 to
2633357
Compare
|
@martint Thanks for the suggestion to put When
So a single assignments map covering both, connectors cannot distinguish which handles belong to which part. That distinction is required in at least these places:
|
Replace the functional predicate in Constraint with ConnectorExpressionEvaluator-based evaluation.
2633357 to
fbabd0b
Compare
Description
Currently,
Constraintmay not fully capture all predicate information required for partition pruning when expressions cannot be translated intoConnectorExpression, and may not able to use if we want to use it on worker(maybe in future).This pr is going to remove the
predicatefrom it and make it finally fully serializable thus keep theConstraintas the single source of truth for pruning-related information.Design
The key idea is to introduce an engine-level abstraction for filter expressions that cannot be represented as ConnectorExpression, without exposing any connector-specific concepts:
Introduce
ConstraintExpressionto represent filter expressions notconvertible to
ConnectorExpressionExpressionand symbol-to-column assignmentsIntroduce
ConstraintExpressionEvaluatortrino-mainRemove
predicatefromConstraintNotes
Constraintmay be evaluated on workersAdditional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: