You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/10/14 04:15:18 UTC
[GitHub] [iceberg] hililiwei opened a new pull request, #5982: Core: Optimize the TableScanContext
hililiwei opened a new pull request, #5982:
URL: https://github.com/apache/iceberg/pull/5982
This PR optimizes the `TableScanContext` to make it easier to read and modify.
Before this PR, if you want to add a field, you need to modify a lot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on pull request #5982: Core: Optimize the TableScanContext
Posted by GitBox <gi...@apache.org>.
hililiwei commented on PR #5982:
URL: https://github.com/apache/iceberg/pull/5982#issuecomment-1280191676
> looks great, but I would rename the copy method from `builder` to `from` as I think this is clearer.
>
> @rdblue / @hililiwei I also created #5985 to have a slightly shorter alternative (171 vs 376 LOC) that doesn't require a custom builder.
Thanks fro your review. #5985 Annotations is really more concise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #5982: Core: Optimize the TableScanContext
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #5982:
URL: https://github.com/apache/iceberg/pull/5982#discussion_r995644673
##########
core/src/main/java/org/apache/iceberg/TableScanContext.java:
##########
@@ -338,40 +196,132 @@ boolean planWithCustomizedExecutor() {
}
TableScanContext planWith(ExecutorService executor) {
- return new TableScanContext(
- snapshotId,
- rowFilter,
- ignoreResiduals,
- caseSensitive,
- colStats,
- projectedSchema,
- selectedColumns,
- options,
- fromSnapshotId,
- toSnapshotId,
- executor,
- fromSnapshotInclusive,
- metricsReporter);
+ return Builder.builder(this).planExecutor(executor).build();
}
MetricsReporter metricsReporter() {
return metricsReporter;
}
TableScanContext reportWith(MetricsReporter reporter) {
- return new TableScanContext(
- snapshotId,
- rowFilter,
- ignoreResiduals,
- caseSensitive,
- colStats,
- projectedSchema,
- selectedColumns,
- options,
- fromSnapshotId,
- toSnapshotId,
- planExecutor,
- fromSnapshotInclusive,
- reporter);
+ return Builder.builder(this).metricsReporter(reporter).build();
+ }
+
+ private static final class Builder {
+ private Long snapshotId;
+ private Expression rowFilter;
+ private boolean ignoreResiduals;
+ private boolean caseSensitive;
+ private boolean colStats;
+ private Schema projectedSchema;
+ private Collection<String> selectedColumns;
+ private ImmutableMap<String, String> options;
+ private Long fromSnapshotId;
+ private Long toSnapshotId;
+ private ExecutorService planExecutor;
+ private boolean fromSnapshotInclusive;
+ private MetricsReporter metricsReporter;
+
+ Builder(TableScanContext context) {
+ this.snapshotId(context.snapshotId)
+ .rowFilter(context.rowFilter)
+ .ignoreResiduals(context.ignoreResiduals)
+ .caseSensitive(context.caseSensitive)
+ .colStats(context.colStats)
+ .projectedSchema(context.projectedSchema)
+ .selectedColumns(context.selectedColumns)
+ .options(context.options)
+ .fromSnapshotId(context.fromSnapshotId)
+ .toSnapshotId(context.toSnapshotId)
+ .planExecutor(context.planExecutor)
+ .fromSnapshotInclusive(context.fromSnapshotInclusive)
+ .metricsReporter(context.metricsReporter);
+ }
+
+ static Builder builder(TableScanContext context) {
Review Comment:
I would probably just call this `from()` rather than `build()` to make it clearer that this is copying state
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 closed pull request #5982: Core: Optimize the TableScanContext
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 closed pull request #5982: Core: Optimize the TableScanContext
URL: https://github.com/apache/iceberg/pull/5982
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] XBaith commented on pull request #5982: Core: Optimize the TableScanContext
Posted by GitBox <gi...@apache.org>.
XBaith commented on PR #5982:
URL: https://github.com/apache/iceberg/pull/5982#issuecomment-1278695117
I believe it's better to move the construction responsibility of the TableScanContext to the inner Builder. Have the builder perform the create operation instead of new TableScanContext().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #5982: Core: Optimize the TableScanContext
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on PR #5982:
URL: https://github.com/apache/iceberg/pull/5982#issuecomment-1524628171
Close with #5985
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a diff in pull request #5982: Core: Optimize the TableScanContext
Posted by GitBox <gi...@apache.org>.
hililiwei commented on code in PR #5982:
URL: https://github.com/apache/iceberg/pull/5982#discussion_r996561783
##########
core/src/main/java/org/apache/iceberg/TableScanContext.java:
##########
@@ -338,40 +196,132 @@ boolean planWithCustomizedExecutor() {
}
TableScanContext planWith(ExecutorService executor) {
- return new TableScanContext(
- snapshotId,
- rowFilter,
- ignoreResiduals,
- caseSensitive,
- colStats,
- projectedSchema,
- selectedColumns,
- options,
- fromSnapshotId,
- toSnapshotId,
- executor,
- fromSnapshotInclusive,
- metricsReporter);
+ return Builder.builder(this).planExecutor(executor).build();
}
MetricsReporter metricsReporter() {
return metricsReporter;
}
TableScanContext reportWith(MetricsReporter reporter) {
- return new TableScanContext(
- snapshotId,
- rowFilter,
- ignoreResiduals,
- caseSensitive,
- colStats,
- projectedSchema,
- selectedColumns,
- options,
- fromSnapshotId,
- toSnapshotId,
- planExecutor,
- fromSnapshotInclusive,
- reporter);
+ return Builder.builder(this).metricsReporter(reporter).build();
+ }
+
+ private static final class Builder {
+ private Long snapshotId;
+ private Expression rowFilter;
+ private boolean ignoreResiduals;
+ private boolean caseSensitive;
+ private boolean colStats;
+ private Schema projectedSchema;
+ private Collection<String> selectedColumns;
+ private ImmutableMap<String, String> options;
+ private Long fromSnapshotId;
+ private Long toSnapshotId;
+ private ExecutorService planExecutor;
+ private boolean fromSnapshotInclusive;
+ private MetricsReporter metricsReporter;
+
+ Builder(TableScanContext context) {
+ this.snapshotId(context.snapshotId)
+ .rowFilter(context.rowFilter)
+ .ignoreResiduals(context.ignoreResiduals)
+ .caseSensitive(context.caseSensitive)
+ .colStats(context.colStats)
+ .projectedSchema(context.projectedSchema)
+ .selectedColumns(context.selectedColumns)
+ .options(context.options)
+ .fromSnapshotId(context.fromSnapshotId)
+ .toSnapshotId(context.toSnapshotId)
+ .planExecutor(context.planExecutor)
+ .fromSnapshotInclusive(context.fromSnapshotInclusive)
+ .metricsReporter(context.metricsReporter);
+ }
+
+ static Builder builder(TableScanContext context) {
Review Comment:
sounds good. Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5982: Core: Optimize the TableScanContext
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5982:
URL: https://github.com/apache/iceberg/pull/5982#issuecomment-1287167796
I don't think that this is worth the change. The context doesn't change very often (this is the 4th change in the 2 years since this was introduced) and I don't think that a builder helps much. In addition, adding a builder is another class to maintain and would be passing state around again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org