korlov42 commented on code in PR #5870:
URL: https://github.com/apache/ignite-3/pull/5870#discussion_r2103988127
##########
modules/api/src/main/java/org/apache/ignite/sql/ColumnType.java:
##########
@@ -150,8 +164,14 @@ public int id() {
return id;
}
- /** Returns corresponding {@code ColumnType} by given id, {@code null} for
unknown id. */
- public static @Nullable ColumnType getById(int id) {
+ /** Returns the {@link ColumnType} instance by its id, or {@code null} if
the id is invalid. */
+ public static @Nullable ColumnType fromId(int id) {
return id >= 0 && id < VALS.length ? VALS[id] : null;
}
+
+ // TODO https://issues.apache.org/jira/browse/IGNITE-17373 Remove filter
after this issue is resolved
+ @TestOnly
Review Comment:
I believe we must not put `@TestOnly` stuff in public api classes and
interfaces
##########
modules/schema/src/testFixtures/java/org/apache/ignite/internal/schema/SchemaTestUtils.java:
##########
@@ -138,7 +138,7 @@ public static Object generateRandomValue(Random rnd,
NativeType type) {
}
/** Creates a native type from given type spec. */
- public static NativeType specToType(NativeTypeSpec spec) {
+ public static NativeType specToType(ColumnType spec) { // todo ????
Review Comment:
> // todo ????
may you elaborate here please what is wrong with this method?
##########
modules/api/src/main/java/org/apache/ignite/sql/ColumnType.java:
##########
@@ -123,6 +128,15 @@ public enum ColumnType {
this.scaleAllowed = scaleDefined;
this.lengthAllowed = lengthDefined;
this.id = id;
+
+ fixedSize = !((precisionAllowed && scaleAllowed) || lengthAllowed);
+ }
+
+ /**
+ * Get fixed length flag: {@code true} for fixed-length types, {@code
false} otherwise.
+ */
+ public boolean fixedLength() {
Review Comment:
`fixedLength` is kinda legacy reminding us about old format. I would prefer
to avoid adding such methods to public api classes since it will be replay hard
to get rid of this. Let's move this method to `NativeType` class
##########
modules/table/src/test/java/org/apache/ignite/internal/table/InteropOperationsTest.java:
##########
@@ -546,8 +546,10 @@ private static class Value {
private UUID fuuidN;
private String fstring;
private String fstringN;
- private byte[] fbytes;
- private byte[] fbytesN;
+ //CHECKSTYLE:OFF
+ private byte[] fbyte_array;
+ private byte[] fbyte_arrayN;
Review Comment:
this refactoring is not really required for this patch, so let's revert it
##########
modules/api/src/main/java/org/apache/ignite/sql/ColumnType.java:
##########
@@ -150,8 +164,14 @@ public int id() {
return id;
}
- /** Returns corresponding {@code ColumnType} by given id, {@code null} for
unknown id. */
- public static @Nullable ColumnType getById(int id) {
+ /** Returns the {@link ColumnType} instance by its id, or {@code null} if
the id is invalid. */
+ public static @Nullable ColumnType fromId(int id) {
Review Comment:
`ColumnType` is part of public api, therefore it's a breaking change. Please
revert it
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/DefaultValue.java:
##########
@@ -138,12 +138,12 @@ public static class ConstantValue extends DefaultValue {
private ConstantValue(@Nullable Object value) {
super(Type.CONSTANT);
- NativeType nativeType = NativeTypes.fromObject(value);
+ @Nullable NativeType type0 = NativeTypes.fromObject(value);
Review Comment:
we don't usually put `@Nullable` for local variables
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/configuration/ValueSerializationHelper.java:
##########
@@ -62,7 +62,7 @@ public static String toString(Object defaultValue, NativeType
type) {
case STRING:
case UUID:
return defaultValue.toString();
Review Comment:
is this helper used anywhere? If not let's just remove 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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
