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/02/16 09:47:46 UTC
[GitHub] [iceberg] yittg opened a new pull request #4140: Refactor TableScanContext to avoid noisy updates while adding field
yittg opened a new pull request #4140:
URL: https://github.com/apache/iceberg/pull/4140
When i am adding field into TableScanContext in #4115 , it would introduce methods updates for others unrelated fields.
Would like to refactor it to keep history clean while adding or removing fields.
cc @rdblue .
--
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] yittg removed a comment on pull request #4140: Refactor TableScanContext to avoid noisy updates while adding field
Posted by GitBox <gi...@apache.org>.
yittg removed a comment on pull request #4140:
URL: https://github.com/apache/iceberg/pull/4140#issuecomment-1042448852
thanks @RussellSpitzer @rdblue .
I got touched this class in #4115 when I want to add an plan executor. and found the noisy update way. to avoid always thinking like ' let's refactor it NEXT time ' and leading to more and more meaningless updates, I tried to do this right now here.
If it's really rarely updated, it's definitely ok to leave it as the way it is and close this PR.
And given it's a simple change, i didn't open an issue in advance to discuss it. Sorry for that.
--
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] yittg commented on pull request #4140: Refactor TableScanContext to avoid noisy updates while adding field
Posted by GitBox <gi...@apache.org>.
yittg commented on pull request #4140:
URL: https://github.com/apache/iceberg/pull/4140#issuecomment-1042448852
thanks @RussellSpitzer @rdblue .
I got touched this class in #4115 when I want to add an plan executor. and found the noisy update way. to avoid always thinking like ' let's refactor it NEXT time ' and leading to more and more meaningless updates, I tried to do this right now here.
If it's really rarely updated, it's definitely ok to leave it as the way it is and close this PR.
And given it's a simple change, i didn't open an issue in advance to discuss it. Sorry for that.
--
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] RussellSpitzer commented on pull request #4140: Refactor TableScanContext to avoid noisy updates while adding field
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #4140:
URL: https://github.com/apache/iceberg/pull/4140#issuecomment-1042189370
This feels like a good idea to me, although I'm hoping we don't have to make that many more additions to TableScanContext. Do you have a lot of fields you are thinking about adding?
--
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] RussellSpitzer commented on a change in pull request #4140: Refactor TableScanContext to avoid noisy updates while adding field
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #4140:
URL: https://github.com/apache/iceberg/pull/4140#discussion_r808420144
##########
File path: core/src/main/java/org/apache/iceberg/TableScanContext.java
##########
@@ -54,10 +54,18 @@
this.toSnapshotId = null;
}
- private TableScanContext(Long snapshotId, Expression rowFilter, boolean ignoreResiduals,
- boolean caseSensitive, boolean colStats, Schema projectedSchema,
- Collection<String> selectedColumns, ImmutableMap<String, String> options,
- Long fromSnapshotId, Long toSnapshotId) {
+ private TableScanContext(
+ Long snapshotId,
Review comment:
While I personally like this format, in Iceberg we stick to the version that was here before with all the args grouped together and left justified with the first arg
--
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] yittg closed pull request #4140: Refactor TableScanContext to avoid noisy updates while adding field
Posted by GitBox <gi...@apache.org>.
yittg closed pull request #4140:
URL: https://github.com/apache/iceberg/pull/4140
--
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] yittg commented on pull request #4140: Refactor TableScanContext to avoid noisy updates while adding field
Posted by GitBox <gi...@apache.org>.
yittg commented on pull request #4140:
URL: https://github.com/apache/iceberg/pull/4140#issuecomment-1042448640
thanks @RussellSpitzer @rdblue .
I got touched this class in #4115 when I want to add an plan executor. and found the noisy update way. to avoid always thinking like ' let's refactor it NEXT time ' and leading to more and more meaningless updates, I tried to do this right now here.
If it's really rarely updated, it's definitely ok to leave it as the way it is and close this PR.
And given it's a simple change, i didn't open an issue in advance to discuss it. Sorry for that.
--
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] yittg commented on pull request #4140: Refactor TableScanContext to avoid noisy updates while adding field
Posted by GitBox <gi...@apache.org>.
yittg commented on pull request #4140:
URL: https://github.com/apache/iceberg/pull/4140#issuecomment-1043765351
closing it.
--
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] RussellSpitzer commented on pull request #4140: Refactor TableScanContext to avoid noisy updates while adding field
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #4140:
URL: https://github.com/apache/iceberg/pull/4140#issuecomment-1042394054
> We rarely change this class, so I'm not sure it's worth all the reformatting and extra builder, but I'm okay with this if @RussellSpitzer is in favor.
I basically feel the same way, unless we had some plans to add a lot more to this class I think it's probably fine the way it is. That's why I asked above what the new planned fields were.
--
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 a change in pull request #4140: Refactor TableScanContext to avoid noisy updates while adding field
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4140:
URL: https://github.com/apache/iceberg/pull/4140#discussion_r808512728
##########
File path: core/src/main/java/org/apache/iceberg/TableScanContext.java
##########
@@ -131,37 +133,124 @@ Schema projectedSchema() {
TableScanContext project(Schema schema) {
Preconditions.checkState(selectedColumns == null, "Cannot set projection schema when columns are selected");
- return new TableScanContext(snapshotId, rowFilter, ignoreResiduals,
- caseSensitive, colStats, schema, null, options, fromSnapshotId, toSnapshotId);
+ return builder().projectedSchema(schema).selectedColumns(null).build();
}
Map<String, String> options() {
return options;
}
TableScanContext withOption(String property, String value) {
- ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
- builder.putAll(options);
- builder.put(property, value);
- return new TableScanContext(snapshotId, rowFilter, ignoreResiduals,
- caseSensitive, colStats, projectedSchema, selectedColumns, builder.build(), fromSnapshotId, toSnapshotId);
+ ImmutableMap.Builder<String, String> mapBuilder = ImmutableMap.builder();
+ mapBuilder.putAll(options);
+ mapBuilder.put(property, value);
Review comment:
This rename looks unnecessary.
--
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 #4140: Refactor TableScanContext to avoid noisy updates while adding field
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #4140:
URL: https://github.com/apache/iceberg/pull/4140#issuecomment-1042378558
We rarely change this class, so I'm not sure it's worth all the reformatting and extra builder, but I'm okay with this if @RussellSpitzer is in favor.
--
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