Skip to content

Add ProblemReport section to metadata schema#3321

Merged
ctubbsii merged 4 commits intoapache:2.1from
ddanielr:feature/3208-add-problem-section
Apr 25, 2023
Merged

Add ProblemReport section to metadata schema#3321
ctubbsii merged 4 commits intoapache:2.1from
ddanielr:feature/3208-add-problem-section

Conversation

@ddanielr
Copy link
Contributor

Adds ProblemReport section to the metadata schema to consolidate metadata table prefixes.
This removes hardcoded prefix strings located in various classes.

Adds ProblemReport section to the metadata schema to consolidate
metadata table prefixes.
Copy link
Contributor

@dlmarion dlmarion 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
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

I wonder if some of these APIs have a String equivalent, so we don't have to wrap them all with new Text. That could be another slight improvement that would go nicely along with this change (if they are equivalent).

Removes Text class use in favor of cleaning up method calls.
@ddanielr
Copy link
Contributor Author

I wonder if some of these APIs have a String equivalent, so we don't have to wrap them all with new Text. That could be another slight improvement that would go nicely along with this change (if they are equivalent).

That's a great idea! I just added the changes to remove the Text wrappers.
Mutation already had a CharSequence method signature to use. I didn't think we needed to add a method for Strings, but happy to do so if that's desired.

I also had to modify the Encoding util to support a byte array for full removal of the Hadoop Text wrappers.

This change seemed minimal as Encoding was only being used by ProblemReport and its test class and immediately generated a byte array from the Text object.

Remove commented code that is not used.
Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

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

One more minor change to eliminate a new Text, but after that, this change looks ready to merge.

*/
public static class ProblemSection {
private static final Section section =
new Section(RESERVED_PREFIX + "err_", true, RESERVED_PREFIX + "err`", false);
Copy link
Member

Choose a reason for hiding this comment

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

This section definition includes the underscore as a mandatory part of the section. That's the most narrowly scoped definition, and completely fine. However, I'm a bit surprised that the section wasn't defined more widely as everything starting with err, as in something like:

      new Section(RESERVED_PREFIX + "err", true, RESERVED_PREFIX + "ers", false);

I am not suggesting changing it... just noting the distinction between the narrow section definition and the wider definition.


if (table == null) {
scanner.setRange(new Range(new Text("~err_"), false, new Text("~err`"), false));
scanner.setRange(ProblemSection.getRange());
Copy link
Member

Choose a reason for hiding this comment

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

One difference is that the new version has the start key inclusive, and the old range has it exclusive. I don't think this matters, since we don't expect any rows to just include the prefix without a tableId after the underscore, so this isn't an important change. I'm just noting it here as a very slight difference in behavior, in case there's a surprise later, and this comment serves to help debug something.

Removes `new Text()` wrappers to clean up code.
@ctubbsii
Copy link
Member

Thanks, @ddanielr . This can be merged once CI checks are finished.

@ctubbsii ctubbsii merged commit 59b9f26 into apache:2.1 Apr 25, 2023
@ddanielr ddanielr deleted the feature/3208-add-problem-section branch April 25, 2023 23:22
@ctubbsii ctubbsii added this to the 2.1.1 milestone Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants