Skip to content
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

Jump to bottom

ORC-1305: Add CompressionWriter and InMemoryEncryption[Reader|Writer] examples #1302

Closed
wants to merge 3 commits into from
Closed

ORC-1305: Add CompressionWriter and InMemoryEncryption[Reader|Writer] examples #1302

wants to merge 3 commits into from

Conversation

deshanxiao
Copy link
Contributor

@deshanxiao deshanxiao commented Nov 1, 2022 •
edited

What changes were proposed in this pull request?

This PR is aimed to add more java examples.

Why are the changes needed?

We need more examples in InMemoryCryption and Compression.

How was this patch tested?

UT

dongjoon-hyun
dongjoon-hyun reviewed Nov 1, 2022
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Could you make the CI happy after checking the result of mvn checkstyle:check -Panalyze -Pbenchmark?

@deshanxiao
Copy link
Contributor Author

Hi @dongjoon-hyun , seem that the CI is successful now. Could you help me take a look at it? Thank you~

guiyanakuang
guiyanakuang approved these changes Nov 2, 2022
View reviewed changes
Copy link
Member

@guiyanakuang guiyanakuang left a comment

Choose a reason for hiding this comment

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

+1 LGTM

@deshanxiao
Copy link
Contributor Author

Thank you @guiyanakuang

dongjoon-hyun
dongjoon-hyun reviewed Nov 2, 2022
View reviewed changes
java/examples/src/java/org/apache/orc/examples/InMemoryCryptionReader.java Outdated
import org.apache.hadoop.hive.ql.exec.vector.BytesColumnVector;
import org.apache.hadoop.hive.ql.exec.vector.LongColumnVector;
import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
import org.apache.orc.*;
Copy link
Member

Choose a reason for hiding this comment

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

We prefer to enumerate the import statements explicitly because most modern IDEs hide this section intelligently.

-import org.apache.orc.*;
+import org.apache.orc.EncryptionAlgorithm;
+import org.apache.orc.InMemoryKeystore;
+import org.apache.orc.OrcFile;
+import org.apache.orc.Reader;
+import org.apache.orc.RecordReader;
+import org.apache.orc.TypeDescription;

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

Let me add these rules to checkstyle.xml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me add these rules to checkstyle.xml.

+1 Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I found that intellij does this "optimization" when it tries to import from the same directory 5 times.

Copy link
Member

Choose a reason for hiding this comment

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

Ya, you need to override it, @deshanxiao .

@dongjoon-hyun dongjoon-hyun added this to the 1.9.0 milestone Nov 2, 2022
wgtmac
wgtmac reviewed Nov 3, 2022
View reviewed changes
java/examples/src/java/org/apache/orc/examples/Driver.java Outdated Show resolved Hide resolved
java/examples/src/java/org/apache/orc/examples/InMemoryCryptionReader.java Outdated
import org.apache.hadoop.hive.ql.exec.vector.BytesColumnVector;
import org.apache.hadoop.hive.ql.exec.vector.LongColumnVector;
import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
import org.apache.orc.*;
Copy link
Member

Choose a reason for hiding this comment

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

+1

@deshanxiao
Copy link
Contributor Author

image

@deshanxiao
Copy link
Contributor Author

Thanks for @guiyanakuang suggestion. I have added the rule. If someone try to use import *, He/She will get an error.

@guiyanakuang
Copy link
Member

Thanks for @guiyanakuang suggestion. I have added the rule. If someone try to use import *, He/She will get an error.

Thank you for doing this, but make sure that each pr has a single purpose. We'd better use another pr to implement it.

@deshanxiao
Copy link
Contributor Author

Thanks, let me remove it.

@dongjoon-hyun
Copy link
Member

Thank you for the update. Could you rebase this PR, @deshanxiao ? I merged the indentation PR to the main branch.

@github-actions github-actions bot added the CPP label Nov 4, 2022
@dongjoon-hyun dongjoon-hyun changed the title ORC-1305: Add more java examples ORC-1305: Add CompressionWriter and InMemoryEncryption[Reader|Writer] examples Nov 6, 2022
dongjoon-hyun
dongjoon-hyun reviewed Nov 6, 2022
View reviewed changes
java/examples/src/java/org/apache/orc/examples/CompressionWriter.java Outdated
public class CompressionWriter {
public static void main(Configuration conf, String[] args) throws IOException {
TypeDescription schema =
TypeDescription.fromString("struct<x:int,y:string>");
Copy link
Member

Choose a reason for hiding this comment

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

Please use 4-space indentation, @deshanxiao .

orc/java/checkstyle.xml

Line 64 in 9cf9d26

<property name="lineWrappingIndentation" value="4"/>

cc @guiyanakuang , the current rule seems to unable to prevent this case because we don't have forceStrictCondition=true.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I didn't notice that. I re-verified the case of forceStrictCondition=true.
orc-core added 1206 Checkstyle violation. Mainly dangling method parameter definitions in strict mode are also treated as violations.
Like this.

  public static Reader createReader(Path path,
                                    ReaderOptions options) throws IOException {
    return new ReaderImpl(path, options);
  }

Copy link
Member

Choose a reason for hiding this comment

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

Ya, I also tried that and found it's too intrusive at this time, @guiyanakuang .

dongjoon-hyun
dongjoon-hyun reviewed Nov 6, 2022
View reviewed changes
java/examples/src/java/org/apache/orc/examples/InMemoryEncryptionWriter.java
import java.io.IOException;
import java.nio.charset.StandardCharsets;

public class InMemoryEncryptionWriter {
Copy link
Member

Choose a reason for hiding this comment

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

The same comments like InMemoryEncryptionReader.

@dongjoon-hyun
Copy link
Member

I finished another round of reviews. Could you check my comments, @deshanxiao ?

@deshanxiao
Copy link
Contributor Author

I finished another round of reviews. Could you check my comments, @deshanxiao ?

Sure, sorry for my format mistakes, I will check it again, thank you.

@deshanxiao
Copy link
Contributor Author

I have done all the comments. Thank you for your very careful review! @dongjoon-hyun

@dongjoon-hyun
Copy link
Member

Thank you for updates, @deshanxiao .

dongjoon-hyun
dongjoon-hyun approved these changes Nov 7, 2022
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM (Pending CIs)

dongjoon-hyun pushed a commit that referenced this pull request Nov 7, 2022
@deshanxiao @dongjoon-hyun
ORC-1305: Add CompressionWriter and `InMemoryEncryption[Reader|Writ…
da67f40 
…er]` examples

### What changes were proposed in this pull request?
This PR is aimed to add more java examples.

### Why are the changes needed?
We need more examples in **InMemoryCryption** and **Compression**.

### How was this patch tested?
UT

Closes #1302 from deshanxiao/deshan/add-more-example.

Authored-by: deshanxiao <deshanxiao@microsoft.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit 31acaaa)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@dongjoon-hyun dongjoon-hyun modified the milestones: 1.9.0, 1.8.1 Nov 7, 2022
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Nov 7, 2022 •
edited

I merged this to main/1.8 for Apache ORC 1.8.1 release because this is an example instead of core module change.

@deshanxiao
Copy link
Contributor Author

Thank you @dongjoon-hyun @guiyanakuang @wgtmac

cxzl25 pushed a commit to cxzl25/orc that referenced this pull request Jan 11, 2024
@deshanxiao @cxzl25
ORC-1305: Add CompressionWriter and `InMemoryEncryption[Reader|Writ…
86ab82e 
…er]` examples

### What changes were proposed in this pull request?
This PR is aimed to add more java examples.

### Why are the changes needed?
We need more examples in **InMemoryCryption** and **Compression**.

### How was this patch tested?
UT

Closes apache#1302 from deshanxiao/deshan/add-more-example.

Authored-by: deshanxiao <deshanxiao@microsoft.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guiyanakuang guiyanakuang guiyanakuang approved these changes

@wgtmac wgtmac wgtmac left review comments

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

Assignees
No one assigned
Labels
BUILD CPP JAVA
Projects
None yet
Milestone
1.8.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@deshanxiao @guiyanakuang @dongjoon-hyun @wgtmac