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

[Kernel] Clean up package organization of kernel-default module. #1954

Merged
merged 1 commit into from
Aug 9, 2023
Merged

[Kernel] Clean up package organization of kernel-default module. #1954

merged 1 commit into from
Aug 9, 2023

Conversation

vkorukanti
Copy link
Collaborator

@vkorukanti vkorukanti commented Aug 1, 2023 •
edited
Loading

Description

Rename the module kernel-default to kernel-defaults.
Rename package io.delta.kernel to io.delta.kernel.defaults in kernel-defaults module.
Move non-public classes under the io.delta.kernel.defaults.internal package.

Note: Reason for not using the default is because it is a keyword in Java and can't be part of the package namespace.

How was this patch tested?

Updated the kernel/examples and ran the python3 kernel/examples/run-kernel-examples.py --use-local successfully.

@vkorukanti vkorukanti requested a review from tdas August 1, 2023 18:47
vkorukanti
vkorukanti commented Aug 1, 2023
View reviewed changes
...l/kernel-default/src/main/java/io/delta/kernel/defaults/client/DefaultExpressionHandler.java Outdated
import io.delta.kernel.expressions.Expression;
import io.delta.kernel.expressions.ExpressionEvaluator;
import io.delta.kernel.expressions.Literal;
import io.delta.kernel.types.*;
import io.delta.kernel.types.BinaryType;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have a checkstyle rule to collapse these together.

@jaceklaskowski
Copy link
Contributor

Please use io.delta.kernel.default (singular not plural) 🙏 That would make more sense to me given the package name is kernel-default. Just like org.apache.spark.sql not org.apache.spark.sqls 😉

@vkorukanti
Copy link
Collaborator Author

Please use io.delta.kernel.default (singular not plural) 🙏 That would make more sense to me given the package name is kernel-default. Just like org.apache.spark.sql not org.apache.spark.sqls 😉

Agree it is big odd, unfortunately default is a keyword in Java and can't be used within the package namespace. Updated the PR to rename the module to kernel-defaults so that they are in sync. Also this module contains default implementations of multiple clients such as DefaultExpressionHandler, DefaultParquetHandler etc. Let me know if there are any issues or better naming alternatives.

allisonport-db
allisonport-db reviewed Aug 8, 2023
View reviewed changes
kernel/examples/table-reader/src/main/java/io/delta/kernel/examples/BaseTableReader.java Outdated
import io.delta.kernel.types.StructField;
import io.delta.kernel.types.StructType;

import static java.lang.String.format;
import static java.util.Objects.requireNonNull;
import io.delta.kernel.defaults.internal.data.vector.VectorUtils;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: internal second?

kernel/kernel-defaults/src/test/java/io/delta/kernel/client/TestDefaultExpressionHandler.java Outdated Show resolved Hide resolved
allisonport-db
allisonport-db reviewed Aug 8, 2023
View reviewed changes
...el/kernel-defaults/src/test/java/io/delta/kernel/defaults/client/TestDefaultJsonHandler.java
@@ -40,7 +42,10 @@
import io.delta.kernel.types.StructType;
import io.delta.kernel.utils.CloseableIterator;
import io.delta.kernel.utils.Utils;
import static io.delta.kernel.utils.DefaultKernelTestUtils.getTestResourceFilePath;

import io.delta.kernel.defaults.utils.DefaultKernelTestUtils;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: no new line? we can't actually enforce this I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I modified the import order in Intellij to remove the blank line.

kernel/kernel-defaults/src/test/java/io/delta/kernel/defaults/integration/BaseIntegration.java Show resolved Hide resolved
.../kernel-defaults/src/test/java/io/delta/kernel/defaults/integration/TestDeltaTableReads.java Show resolved Hide resolved
kernel/kernel-defaults/src/test/scala/io/delta/kernel/DeletionVectorSuite.scala Outdated Show resolved Hide resolved
allisonport-db
allisonport-db approved these changes Aug 8, 2023
View reviewed changes
Copy link
Collaborator

@allisonport-db allisonport-db left a comment

Choose a reason for hiding this comment

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

LGTM

@vkorukanti vkorukanti merged commit efa566f into delta-io:master Aug 9, 2023
6 checks passed
@vkorukanti vkorukanti deleted the cleanup branch September 14, 2023 11:55
@vkorukanti vkorukanti mentioned this pull request Sep 28, 2023
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allisonport-db allisonport-db allisonport-db approved these changes

@tdas tdas Awaiting requested review from tdas

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vkorukanti @jaceklaskowski @allisonport-db