Skip to content

Removed table name and id from RFileScanner. #5512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dlmarion merged 2 commits into from
Apr 28, 2025
Merged

Removed table name and id from RFileScanner. #5512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c1e2a0
Removed table name and id from RFileScanner.
dlmarion Apr 28, 2025
f857cb0
Support null tableId to avoid throwing exception
dlmarion Apr 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,7 @@ public RFileScannerEnvironmentImpl(Opts opts) {

@Override
public String getTableName(TableId tableId) throws TableNotFoundException {
Preconditions.checkArgument(tableId == TABLE_ID, "Expected " + TABLE_ID + " obtained"
+ " from IteratorEnvironment.getTableId(), but got: " + tableId);
return TABLE_NAME;
return null;
}

@Override
Expand All @@ -107,17 +105,13 @@ public Configuration getConfiguration() {

@Override
public Configuration getConfiguration(TableId tableId) {
Preconditions.checkArgument(tableId == TABLE_ID, "Expected " + TABLE_ID + " obtained"
+ " from IteratorEnvironment.getTableId(), but got: " + tableId);
return tableConf;
}

}

private static final byte[] EMPTY_BYTES = new byte[0];
private static final Range EMPTY_RANGE = new Range();
private static final String TABLE_NAME = "rfileScanner";
private static final TableId TABLE_ID = TableId.of(TABLE_NAME);
Copy link
Member

@kevinrr888 kevinrr888 Apr 28, 2025

Choose a reason for hiding this comment

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

could have TABLE_ID = null and check that argument for getConfiguration and getTableName is null/TABLE_ID to avoid accepting random values. Argument should be env.getTableId()

Copy link
Contributor Author

@dlmarion dlmarion Apr 28, 2025

Choose a reason for hiding this comment

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

The RFileScanner is client utility where the user supplies the set of input files, configuration, columns they want to fetch, etc. I don't know that there is utility in validating the table id when this is being used outside of a table context.

Copy link
Member

@kevinrr888 kevinrr888 Apr 28, 2025

Choose a reason for hiding this comment

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

Yeah, this is a weird case where we need to pretend there is a table id when there isn't. I'm fine also not validating it, my idea was just that we should expect env.getTableId() as the way of getting the table id when it is needed instead of accepting any table id

Copy link
Contributor Author

@dlmarion dlmarion Apr 28, 2025

Choose a reason for hiding this comment

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

Does validating the table id and throwing an exception change the behavior of the code between the 2.1.3 and 2.1.4 releases? Are we going to introduce an error into the clients use of our utility by upgrading?

Copy link
Contributor Author

@dlmarion dlmarion Apr 28, 2025

Choose a reason for hiding this comment

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

Answering my own question - Looking at the 2.1.3 RFileScanner code, IterEnv doesn't override getServiceEnv, getPluginEnv or getTableId so any usage of those methods would throw an UnsupportedOperationException. We could add the validation, or not, it doesn't matter as the methods aren't called in RFileScanner.


private Range range;
private BlockCacheManager blockCacheManager = null;
Expand Down Expand Up @@ -309,7 +303,7 @@ public Iterator<Entry<Key,Value>> iterator() {

ClientIteratorEnvironment.Builder iterEnvBuilder = new ClientIteratorEnvironment.Builder()
.withEnvironment(new RFileScannerEnvironmentImpl(opts)).withAuthorizations(opts.auths)
.withScope(IteratorScope.scan).withTableId(TABLE_ID);
.withScope(IteratorScope.scan).withTableId(null);
if (getSamplerConfiguration() != null) {
iterEnvBuilder.withSamplerConfiguration(getSamplerConfiguration());
iterEnvBuilder.withSamplingEnabled();
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/accumulo/core/iteratorsImpl/ClientIteratorEnvironment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public Builder isUserCompaction() {

public Builder withTableId(TableId tableId) {
checkState(this.tableId.isEmpty(), "TableId has already been set");
this.tableId = Optional.of(tableId);
this.tableId = Optional.ofNullable(tableId);
return this;
}

Expand Down