You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2022/09/19 13:56:26 UTC
[GitHub] [hive] difin opened a new pull request, #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
difin opened a new pull request, #3606:
URL: https://github.com/apache/hive/pull/3606
…ethod.
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
difin commented on PR #3606:
URL: https://github.com/apache/hive/pull/3606#issuecomment-1252466669
> Is the jira reference correct; I don't understand wha'ts the connection with [HIVE-26496](https://issues.apache.org/jira/browse/HIVE-26496).
The connection to HIVE-26496 is that you added the suggestion about
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3606:
URL: https://github.com/apache/hive/pull/3606#issuecomment-1251200951
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3606)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=BUG) [2 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3606&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3606&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3606&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=CODE_SMELL) [8 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3606&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3606&metric=duplicated_lines_density&view=list) No Duplication information
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3606:
URL: https://github.com/apache/hive/pull/3606#issuecomment-1252720691
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3606)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=BUG) [2 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3606&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3606&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3606&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=CODE_SMELL) [9 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3606&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3606&metric=duplicated_lines_density&view=list) No Duplication information
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] ayushtkn commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
ayushtkn commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r979098870
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -384,7 +384,6 @@ public void parse(Configuration conf, Path rootPath) throws IOException {
OrcRawRecordMerger.TransactionMetaData.findWriteIDForSynthetcRowIDs(getPath(), rootPath, conf);
writeId = tmd.syntheticWriteId;
stmtId = tmd.statementId;
- bucketId = AcidUtils.parseBucketId(getPath());
Review Comment:
I am not very sure about this part of code. But just going through the code. the ``bucketId= AcidUtils.parseBucketId(getPath());``, the Path can change also if someone calls ``readFields``. It does this in the super class:
```
public void readFields(DataInput in) throws IOException {
file = new Path(Text.readString(in));
```
the `file` is what returns from ``getPath()``, So, I am not sure if it is a good idea to just have that set in just the constructor.
Moreover if any other change in Hadoop comes, which bothers the path in some other method post the construction invocation, again we will be prone to may be some silent errors.
Second, there is a legacy protected constructor which doesn't have path and expect ``readFields`` to do the magic for it, if some thirdparty application/project is still using that by any means, that will get screwed, So, we have to remove that constructor also I feel.
Would require someone more experienced to tell, how safe this is
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r981209262
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -384,7 +384,6 @@ public void parse(Configuration conf, Path rootPath) throws IOException {
OrcRawRecordMerger.TransactionMetaData.findWriteIDForSynthetcRowIDs(getPath(), rootPath, conf);
writeId = tmd.syntheticWriteId;
stmtId = tmd.statementId;
- bucketId = AcidUtils.parseBucketId(getPath());
Review Comment:
No problem at all! This way I learned something new.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r975377018
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -103,7 +103,7 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- int bucketId = AcidUtils.parseBucketId(path);
+ bucketId = AcidUtils.parseBucketId(path);
Review Comment:
I didn't find explanation in history why bucket id was set in parse method. Regarding making bucketId final: bucketId is a private class member and cannot be set outside of OrcSplit. Before my change it was set in parse() method using as input the getPath() method from its parent class. This method uses 'fs' class member from the parent class which is package-private and unaccessible from OrcSplit. So, bucketId is set using path which cannot be modified and it can have only one value. Therefore in my opinion bucketId can be set final and it is safe to do. In other words, if bucketId is set in constructor or in parse method, it can be assigned only same unique value which is based on the path and the path can't be changed after initial assignment in the constructor.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r975377018
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -103,7 +103,7 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- int bucketId = AcidUtils.parseBucketId(path);
+ bucketId = AcidUtils.parseBucketId(path);
Review Comment:
I didn't find explanation in history why bucket id was set in parse method. Regarding making bucketId final: bucketId is a private class member and cannot be set outside of OrcSplit. Before my change it was set using getPath() method from its parent class. This method uses 'fs' class member from the parent class which is package-private and unaccessible from OrcSplit. So, bucketId is set using path which cannot be modified and it can have only one value. Therefore in my opinion bucketId can be set final and it is safe to do. In other words, if bucketId is set in constructor or in parse method, it can be assigned only same unique value which is based on the path and the path can't be changed at runtime.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r975377018
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -103,7 +103,7 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- int bucketId = AcidUtils.parseBucketId(path);
+ bucketId = AcidUtils.parseBucketId(path);
Review Comment:
I didn't find explanation in history why bucket id was set in parse method. Regarding making bucketId final: bucketId is a private class member and cannot be set outside of OrcSplit. Before my change it was set in parse() method using as input the getPath() method from its parent class. This method uses 'fs' class member from the parent class which is package-private and unaccessible from OrcSplit. So, bucketId is set using path which cannot be modified and it can have only one value. Therefore in my opinion bucketId can be set final and it is safe to do. In other words, if bucketId is set in constructor or in parse method, it can be assigned only same unique value which is based on the path and the path can't be changed at runtime.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r980052504
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -384,7 +384,6 @@ public void parse(Configuration conf, Path rootPath) throws IOException {
OrcRawRecordMerger.TransactionMetaData.findWriteIDForSynthetcRowIDs(getPath(), rootPath, conf);
writeId = tmd.syntheticWriteId;
stmtId = tmd.statementId;
- bucketId = AcidUtils.parseBucketId(getPath());
Review Comment:
Thanks for the code review! I didn't think that this class may be serizlized/deserialized and bucketId may need to be set after deserialization in the parse method. Will close this PR.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r978943267
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -384,7 +384,6 @@ public void parse(Configuration conf, Path rootPath) throws IOException {
OrcRawRecordMerger.TransactionMetaData.findWriteIDForSynthetcRowIDs(getPath(), rootPath, conf);
writeId = tmd.syntheticWriteId;
stmtId = tmd.statementId;
- bucketId = AcidUtils.parseBucketId(getPath());
Review Comment:
As I understand we always pass through constructor which sets bucketId. The class has 2 constructors and the bucket Id is set in both of them. Otherwise it wouldn't compile, I made bucketId final and final members must be set before the end of a constructor.
writeId and stmtId depend also on configuration and rootPath which are passed as arguments to parse method, but bucket Id was depending only on path.
https://github.com/apache/hive/pull/3606/files#diff-f33c72f4823ff07b5fb662959932648b9e96c52f248e44df29bd4e8d550487b2R383-R387:~:text=TransactionMetaData%20tmd%20%3D-,OrcRawRecordMerger,-.TransactionMetaData.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r975377018
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -103,7 +103,7 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- int bucketId = AcidUtils.parseBucketId(path);
+ bucketId = AcidUtils.parseBucketId(path);
Review Comment:
I didn't find explanation in history why bucket id was set in parse method. Regarding making bucketId final: bucketId is a private class member and cannot be set outside of OrcSplit. Before my change it was set in parse() method using as input the getPath() method from its parent class. This method uses 'fs' class member from the parent class which is package-private and unaccessible from OrcSplit. So, bucketId is set using path which cannot be modified and it can have only one value. Therefore in my opinion bucketId can be set final and it is safe to do. In other words, if bucketId is set in constructor or in parse method, it can be assigned only same single value which is based on the path and the path can't be changed after initial assignment in the constructor.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
zabetak commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r978725482
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -384,7 +384,6 @@ public void parse(Configuration conf, Path rootPath) throws IOException {
OrcRawRecordMerger.TransactionMetaData.findWriteIDForSynthetcRowIDs(getPath(), rootPath, conf);
writeId = tmd.syntheticWriteId;
stmtId = tmd.statementId;
- bucketId = AcidUtils.parseBucketId(getPath());
Review Comment:
Yes but do we *always* pass through the constructor which sets the `bucketId`?
The `*Split` classes are serializable which makes me a bit skeptical about which constructor is called at each point.
Moreover, `writeId` and `stmtId` are also set here and they seem to depend exclusively on `getPath()` which again makes me a bit suspicious why they were not set in constructor if was possible to do so.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
zabetak commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r978727906
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -384,7 +384,6 @@ public void parse(Configuration conf, Path rootPath) throws IOException {
OrcRawRecordMerger.TransactionMetaData.findWriteIDForSynthetcRowIDs(getPath(), rootPath, conf);
writeId = tmd.syntheticWriteId;
stmtId = tmd.statementId;
- bucketId = AcidUtils.parseBucketId(getPath());
Review Comment:
Hey @ayushtkn are you familiar with `InputSplit` and `InputFormat`? Do you know if the refactoring here is safe?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin closed pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
difin closed pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
URL: https://github.com/apache/hive/pull/3606
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3606:
URL: https://github.com/apache/hive/pull/3606#issuecomment-1253003058
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3606)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=BUG) [2 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3606&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3606&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3606&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=CODE_SMELL) [14 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3606&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3606&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3606&metric=duplicated_lines_density&view=list) No Duplication information
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
zabetak commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r975115739
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -384,7 +384,6 @@ public void parse(Configuration conf, Path rootPath) throws IOException {
OrcRawRecordMerger.TransactionMetaData.findWriteIDForSynthetcRowIDs(getPath(), rootPath, conf);
writeId = tmd.syntheticWriteId;
stmtId = tmd.statementId;
- bucketId = AcidUtils.parseBucketId(getPath());
Review Comment:
Is it safe to remove this? Do we have test coverage around this method (directly or indirectly)?
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -103,7 +103,7 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- int bucketId = AcidUtils.parseBucketId(path);
+ bucketId = AcidUtils.parseBucketId(path);
Review Comment:
If we only set `bucketId` here then it can be made final but is it safe to do so? I am skeptical cause so far we were setting this variable in the `parse` method. Maybe there is something in the history explaining what it was like 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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
zabetak commented on PR #3606:
URL: https://github.com/apache/hive/pull/3606#issuecomment-1252564642
I completely forgot that I commented under the PR for HIVE-26496; sorry about that. Many thanks for the clarification and preparing the follow-up!
The fact that bucketId is not assigned in the constructor is not something that was caused by https://github.com/apache/hive/pull/3559; it was there before that so it is not really tight to HIVE-26496.
This is a beneficial refactoring that makes sense with or without HIVE-26496 so it could have a JIRA on its own.
Other than that I think it is fine to commit under the same JIRA id although I don't see this pattern very often in the project.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r975463464
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -384,7 +384,6 @@ public void parse(Configuration conf, Path rootPath) throws IOException {
OrcRawRecordMerger.TransactionMetaData.findWriteIDForSynthetcRowIDs(getPath(), rootPath, conf);
writeId = tmd.syntheticWriteId;
stmtId = tmd.statementId;
- bucketId = AcidUtils.parseBucketId(getPath());
Review Comment:
Previously bucketId was set in parse() method. Now bucketId is set in constructor. Both assign the same value.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
zabetak commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r979867090
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -384,7 +384,6 @@ public void parse(Configuration conf, Path rootPath) throws IOException {
OrcRawRecordMerger.TransactionMetaData.findWriteIDForSynthetcRowIDs(getPath(), rootPath, conf);
writeId = tmd.syntheticWriteId;
stmtId = tmd.statementId;
- bucketId = AcidUtils.parseBucketId(getPath());
Review Comment:
If we mark a `transient` field as `final` we are basically breaking the class since we can no longer set the value of the `bucketId` field during deserialization.
It may be fine to set `bucketId` in the constructor but I am pretty sure we cannot remove this line from the `parse` method.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a diff in pull request #3606: HIVE-26496: Populating bucketId in the constructor instead of parse m…
Posted by GitBox <gi...@apache.org>.
zabetak commented on code in PR #3606:
URL: https://github.com/apache/hive/pull/3606#discussion_r980428925
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -384,7 +384,6 @@ public void parse(Configuration conf, Path rootPath) throws IOException {
OrcRawRecordMerger.TransactionMetaData.findWriteIDForSynthetcRowIDs(getPath(), rootPath, conf);
writeId = tmd.syntheticWriteId;
stmtId = tmd.statementId;
- bucketId = AcidUtils.parseBucketId(getPath());
Review Comment:
Really sorry for making you invest time on this; I didn't foresee all this when I made the initial comment.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org