You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2026/02/13 16:14:44 UTC
[PR] Convert classes to records [polaris]
adutra opened a new pull request, #3758:
URL: https://github.com/apache/polaris/pull/3758
Now that we have Java 17 on client modules, this PR takes a few good candidates for record classes and migrates them.
Whenever appropriate, the new "builder for records" feature by the Java Immutables library was used as well.
No functional change.
## Checklist
- [ ] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [ ] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [ ] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] ๐ก Added comments for complex logic
- [ ] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Convert classes to records [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3758:
URL: https://github.com/apache/polaris/pull/3758#discussion_r2804995035
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java:
##########
@@ -45,47 +45,7 @@
*/
public class PolarisEntity extends PolarisBaseEntity {
- public static class NameAndId {
- private final String name;
- private final long id;
-
- public NameAndId(String name, long id) {
- this.name = name;
- this.id = id;
- }
-
- public String getName() {
- return name;
- }
-
- public long getId() {
- return id;
- }
- }
-
- public static class TypeSubTypeAndName {
Review Comment:
This one was unused, so I removed.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Convert classes to records [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3758:
URL: https://github.com/apache/polaris/pull/3758#discussion_r2805055393
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java:
##########
@@ -118,38 +118,5 @@ public abstract StorageAccessConfig getSubscopedCreds(
* Result of calling {@link #validateAccessToLocations(RealmConfig,
* PolarisStorageConfigurationInfo, Set, Set)}
*/
- public static final class ValidationResult {
- private final boolean success;
- private final String message;
-
- public ValidationResult(boolean success, String message) {
- this.success = success;
- this.message = message;
- }
-
- public boolean isSuccess() {
- return success;
- }
-
- public String getMessage() {
- return message;
- }
-
- @Override
- public boolean equals(Object o) {
Review Comment:
This was an interesting case: the equals/hashCode methods were considering only `success`, not `message`. But this doesn't feel right.
Moreover, instances of this class are never compared, or added to sets or map keys. The actual equals/hashCode implementation seems irrelevant.
It turns out, this equals/hashCode implementation was probably meant for tests: some tests compare instances by ignoring the message.
I fixed the tests instead.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Convert classes to records [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #3758:
URL: https://github.com/apache/polaris/pull/3758#issuecomment-3926801495
If there are no objections, I'm going to merge this by EOD today.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Convert classes to records [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra merged PR #3758:
URL: https://github.com/apache/polaris/pull/3758
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Convert classes to records [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3758:
URL: https://github.com/apache/polaris/pull/3758#discussion_r2804998952
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/ResolverEntityName.java:
##########
@@ -18,49 +18,24 @@
*/
package org.apache.polaris.core.persistence.resolver;
-import java.util.Objects;
import org.apache.polaris.core.entity.PolarisEntityType;
+import org.apache.polaris.immutables.PolarisImmutable;
+import org.immutables.value.Value;
/** Simple class to represent the name of an entity to resolve */
-public class ResolverEntityName {
+@PolarisImmutable
+public interface ResolverEntityName {
Review Comment:
This one was converted to a `PolarisImmutable` instead, because it requires a special equals/hashCode implementation.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Convert classes to records [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #3758:
URL: https://github.com/apache/polaris/pull/3758#discussion_r2805040932
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/ResolverPath.java:
##########
@@ -18,76 +18,28 @@
*/
package org.apache.polaris.core.persistence.resolver;
-import com.google.common.collect.ImmutableList;
import java.util.List;
import org.apache.polaris.core.entity.PolarisEntityType;
-/** Simple class to represent a path within a catalog */
-public class ResolverPath {
-
- // name of the entities in that path. The parent of the first named entity is the path is the
- // catalog
- private final List<String> entityNames;
-
- // all entities in a path are namespaces except the last one which can be a table_like entity
- // versus a namespace
- private final PolarisEntityType lastEntityType;
-
- // true if this path is optional, i.e. failing to fully resolve it is not an error
- private final boolean isOptional;
-
- /**
- * Constructor for an optional path
- *
- * @param entityNames set of entity names, all are namespaces except the last one a namespa
- */
- public ResolverPath(List<String> entityNames) {
Review Comment:
This constructor was unused, so I removed it.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org