You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "lowka (via GitHub)" <gi...@apache.org> on 2024/04/18 12:59:35 UTC
[PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
lowka opened a new pull request, #3627:
URL: https://github.com/apache/ignite-3/pull/3627
Removes serialization from `DefaultValue`.
https://issues.apache.org/jira/browse/IGNITE-21435
---
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan merged PR #3627:
URL: https://github.com/apache/ignite-3/pull/3627
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #3627:
URL: https://github.com/apache/ignite-3/pull/3627#discussion_r1572613224
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/storage/CatalogEntrySerializationTest.java:
##########
@@ -144,6 +174,85 @@ void test(MarshallableEntryType type) {
}
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("values")
+ public void constantDefault(ColumnType columnType, Object value) throws IOException {
+ ConstantValue val = (ConstantValue) DefaultValue.constant(value);
+
+ log.info("{}: {}", columnType, value);
+
+ try (IgniteUnsafeDataOutput os = new IgniteUnsafeDataOutput(128)) {
+ DefaultValue.writeTo(val, os);
+
+ try (IgniteUnsafeDataInput in = new IgniteUnsafeDataInput(os.internalArray())) {
+ DefaultValue actual = DefaultValue.readFrom(in);
+ assertEquals(val, actual);
+ }
+ }
+ }
+
+ private static Stream<Arguments> values() {
+ List<Object> list = new ArrayList<>();
+
+ list.add(null);
+ list.add(RND.nextBoolean());
+
+ list.add((byte) RND.nextInt());
+ list.add((short) RND.nextInt());
+ list.add(RND.nextInt());
+ list.add(RND.nextLong());
+ list.add((float) RND.nextDouble());
+ list.add(RND.nextDouble());
+
+ list.add(BigDecimal.valueOf(RND.nextLong()));
+ list.add(BigDecimal.valueOf(RND.nextLong(), RND.nextInt(100)));
+
+ list.add(BigInteger.valueOf(RND.nextLong()));
+
+ list.add(LocalTime.of(RND.nextInt(24), RND.nextInt(60), RND.nextInt(60), RND.nextInt(100_000)));
+ list.add(LocalDate.of(RND.nextInt(4000) - 1000, RND.nextInt(12) + 1, RND.nextInt(27) + 1));
+ list.add(LocalDateTime.of(
+ LocalDate.of(RND.nextInt(4000) - 1000, RND.nextInt(12) + 1, RND.nextInt(27) + 1),
+ LocalTime.of(RND.nextInt(24), RND.nextInt(60), RND.nextInt(60), RND.nextInt(100_000))
+ ));
+
+ byte[] bytes = new byte[RND.nextInt(1000)];
+ RND.nextBytes(bytes);
+ list.add(Base64.getEncoder().encodeToString(bytes));
+
+ list.add(UUID.randomUUID());
+
+ // TODO Include ignored values to test after https://issues.apache.org/jira/browse/IGNITE-15200
+ // list.add(Duration.of(11, ChronoUnit.HOURS));
+ // list.add(Period.of(5, 4, 3));
+
+ BitSet bitSet = new BitSet();
+ for (int i = 0; i < RND.nextInt(100); i++) {
+ int b = RND.nextInt(1024);
+ bitSet.set(b);
+ }
+ list.add(bitSet);
+
+ return list.stream().map(val -> {
+ NativeType nativeType = NativeTypes.fromObject(val);
+ return Arguments.of(nativeType == null ? ColumnType.NULL : nativeType.spec().asColumnType(), val);
+ });
+ }
+
+ @Test
+ public void functionCallDefault() throws IOException{
Review Comment:
Added prefix.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #3627:
URL: https://github.com/apache/ignite-3/pull/3627#discussion_r1572230375
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/storage/CatalogEntrySerializationTest.java:
##########
@@ -144,6 +174,85 @@ void test(MarshallableEntryType type) {
}
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("values")
+ public void constantDefault(ColumnType columnType, Object value) throws IOException {
Review Comment:
```suggestion
public void testConstantDefaultAllTypes(ColumnType columnType, Object value) throws IOException {
```
:thinking:
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #3627:
URL: https://github.com/apache/ignite-3/pull/3627#discussion_r1571801388
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/storage/CatalogEntrySerializationTest.java:
##########
@@ -45,20 +59,36 @@
import org.apache.ignite.internal.catalog.storage.serialization.MarshallableEntryType;
import org.apache.ignite.internal.catalog.storage.serialization.UpdateLogMarshallerImpl;
import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
-import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.type.NativeType;
+import org.apache.ignite.internal.type.NativeTypes;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput;
import org.apache.ignite.sql.ColumnType;
import org.assertj.core.api.BDDAssertions;
import org.jetbrains.annotations.Nullable;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.EnumSource;
import org.junit.jupiter.params.provider.EnumSource.Mode;
+import org.junit.jupiter.params.provider.MethodSource;
/**
* Tests to verify catalog storage entries serialization.
*/
public class CatalogEntrySerializationTest extends BaseIgniteAbstractTest {
+ private static final long seed = System.nanoTime();
+
+ private static final Random RND = new Random(seed);
+
private final UpdateLogMarshallerImpl marshaller = new UpdateLogMarshallerImpl();
+ @BeforeEach
+ public void setup() {
+ log.info("Seed: {}", seed);
Review Comment:
Done.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #3627:
URL: https://github.com/apache/ignite-3/pull/3627#discussion_r1571796956
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/DefaultValue.java:
##########
@@ -68,9 +92,28 @@ public Type type() {
return type;
}
+ /** Serializes this default value or {@code null}. */
+ public abstract void writeTo(IgniteDataOutput os) throws IOException;
+
+ /** Reads default value or {@code null}. */
+ public static @Nullable DefaultValue readFrom(IgniteDataInput in) throws IOException {
+ int typeId = in.readByte();
+ if (typeId == Type.NO_DEFAULT) {
+ return null;
+ } else if (typeId == Type.CONSTANT.typeId) {
+ Object val = readValue(in);
+ return new ConstantValue(val);
+ } else if (typeId == Type.FUNCTION_CALL.typeId) {
+ String functionName = in.readUTF();
+ return new FunctionCall(functionName);
+ } else {
+ throw new IllegalArgumentException("Unexpected type: " + typeId);
Review Comment:
You can not use references in case clause.
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/DefaultValue.java:
##########
@@ -68,9 +92,28 @@ public Type type() {
return type;
}
+ /** Serializes this default value or {@code null}. */
+ public abstract void writeTo(IgniteDataOutput os) throws IOException;
+
+ /** Reads default value or {@code null}. */
+ public static @Nullable DefaultValue readFrom(IgniteDataInput in) throws IOException {
+ int typeId = in.readByte();
+ if (typeId == Type.NO_DEFAULT) {
+ return null;
+ } else if (typeId == Type.CONSTANT.typeId) {
+ Object val = readValue(in);
+ return new ConstantValue(val);
+ } else if (typeId == Type.FUNCTION_CALL.typeId) {
+ String functionName = in.readUTF();
+ return new FunctionCall(functionName);
+ } else {
+ throw new IllegalArgumentException("Unexpected type: " + typeId);
Review Comment:
You cannot use references in case clause.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #3627:
URL: https://github.com/apache/ignite-3/pull/3627#discussion_r1571901207
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/DefaultValue.java:
##########
@@ -51,10 +65,20 @@ public static DefaultValue constant(@Nullable Object value) {
/** Types of the defaults. */
public enum Type {
/** Default is specified as a constant. */
- CONSTANT,
+ CONSTANT(0),
/** Default is specified as a call to a function. */
- FUNCTION_CALL
+ FUNCTION_CALL(1);
+
+ /** Represents absent of default value ({@code null}). */
+ private static final int NO_DEFAULT = -1;
+
+ /** type id used by serialization. */
Review Comment:
Thanks.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #3627:
URL: https://github.com/apache/ignite-3/pull/3627#discussion_r1571197859
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/IgniteDataOutput.java:
##########
@@ -129,6 +139,86 @@ public interface IgniteDataOutput extends DataOutput {
*/
void writeCharArray(char[] arr) throws IOException;
+ /**
+ * Writes big integer.
+ *
+ * @param val Big integer.
+ * @throws IOException In case of error.
+ */
+ void writeBigInteger(BigInteger val) throws IOException;
+
+ /**
+ * Writes decimal.
+ *
+ * @param val Decimal.
Review Comment:
```suggestion
* Writes big decimal.
*
* @param val Big decimal.
```
for consistency with `readBigDecimal` :sunglasses:
##########
modules/core/src/test/java/org/apache/ignite/internal/util/io/IgniteUnsafeDataInputOutputByteOrderTest.java:
##########
@@ -237,4 +249,101 @@ public void testDoubleArray() throws Exception {
assertArrayEquals(arr, in.readDoubleArray(ARR_LEN), 0);
}
+
+ @Test
+ public void testLocalTime() throws IOException {
+ LocalTime val = LocalTime.of(RND.nextInt(24), RND.nextInt(60), RND.nextInt(60), RND.nextInt(10000));
+
+ out.writeLocalTime(val);
+
+ assertEquals(val, in.readLocalTime());
+ }
+
+ @Test
+ public void testLocalDate() throws IOException {
+ LocalDate val = LocalDate.of(RND.nextInt(500) + 1900, RND.nextInt(12) + 1, 1 + RND.nextInt(27));
+
+ out.writeLocalDate(val);
+
+ assertEquals(val, in.readLocalDate());
+ }
+
+ @Test
+ public void testLocalDateTime() throws IOException {
+ LocalTime time = LocalTime.of(RND.nextInt(24), RND.nextInt(60), RND.nextInt(60), RND.nextInt(10000));
+ LocalDate date = LocalDate.of(RND.nextInt(500) + 1900, RND.nextInt(12) + 1, 1 + RND.nextInt(27));
Review Comment:
```suggestion
LocalDate date = LocalDate.of(RND.nextInt(4000) - 1000, RND.nextInt(12) + 1, 1 + RND.nextInt(27));
```
##########
modules/core/src/test/java/org/apache/ignite/internal/util/io/IgniteUnsafeDataInputOutputByteOrderTest.java:
##########
@@ -237,4 +249,101 @@ public void testDoubleArray() throws Exception {
assertArrayEquals(arr, in.readDoubleArray(ARR_LEN), 0);
}
+
+ @Test
+ public void testLocalTime() throws IOException {
+ LocalTime val = LocalTime.of(RND.nextInt(24), RND.nextInt(60), RND.nextInt(60), RND.nextInt(10000));
+
+ out.writeLocalTime(val);
+
+ assertEquals(val, in.readLocalTime());
+ }
+
+ @Test
+ public void testLocalDate() throws IOException {
+ LocalDate val = LocalDate.of(RND.nextInt(500) + 1900, RND.nextInt(12) + 1, 1 + RND.nextInt(27));
+
+ out.writeLocalDate(val);
+
+ assertEquals(val, in.readLocalDate());
+ }
+
+ @Test
+ public void testLocalDateTime() throws IOException {
+ LocalTime time = LocalTime.of(RND.nextInt(24), RND.nextInt(60), RND.nextInt(60), RND.nextInt(10000));
+ LocalDate date = LocalDate.of(RND.nextInt(500) + 1900, RND.nextInt(12) + 1, 1 + RND.nextInt(27));
+ LocalDateTime val = LocalDateTime.of(date, time);
+
+ out.writeLocalDateTime(val);
+
+ assertEquals(val, in.readLocalDateTime());
+ }
+
+ @Test
+ public void testInstant() throws IOException {
+ Instant val = Instant.ofEpochMilli(Math.abs(RND.nextLong()));
Review Comment:
```suggestion
Instant val = Instant.ofEpochMilli(RND.nextLong());
```
With Math.abs, this will only cover dates starting from 1970.
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/storage/CatalogEntrySerializationTest.java:
##########
@@ -45,20 +59,36 @@
import org.apache.ignite.internal.catalog.storage.serialization.MarshallableEntryType;
import org.apache.ignite.internal.catalog.storage.serialization.UpdateLogMarshallerImpl;
import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
-import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.type.NativeType;
+import org.apache.ignite.internal.type.NativeTypes;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataInput;
+import org.apache.ignite.internal.util.io.IgniteUnsafeDataOutput;
import org.apache.ignite.sql.ColumnType;
import org.assertj.core.api.BDDAssertions;
import org.jetbrains.annotations.Nullable;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.EnumSource;
import org.junit.jupiter.params.provider.EnumSource.Mode;
+import org.junit.jupiter.params.provider.MethodSource;
/**
* Tests to verify catalog storage entries serialization.
*/
public class CatalogEntrySerializationTest extends BaseIgniteAbstractTest {
+ private static final long seed = System.nanoTime();
+
+ private static final Random RND = new Random(seed);
+
private final UpdateLogMarshallerImpl marshaller = new UpdateLogMarshallerImpl();
+ @BeforeEach
+ public void setup() {
+ log.info("Seed: {}", seed);
Review Comment:
Could you add the same initialization to `IgniteUnsafeDataInputOutputByteOrderTest` if it doesn't bother you?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/DefaultValue.java:
##########
@@ -68,9 +92,28 @@ public Type type() {
return type;
}
+ /** Serializes this default value or {@code null}. */
+ public abstract void writeTo(IgniteDataOutput os) throws IOException;
+
+ /** Reads default value or {@code null}. */
+ public static @Nullable DefaultValue readFrom(IgniteDataInput in) throws IOException {
+ int typeId = in.readByte();
+ if (typeId == Type.NO_DEFAULT) {
+ return null;
+ } else if (typeId == Type.CONSTANT.typeId) {
+ Object val = readValue(in);
+ return new ConstantValue(val);
+ } else if (typeId == Type.FUNCTION_CALL.typeId) {
+ String functionName = in.readUTF();
+ return new FunctionCall(functionName);
+ } else {
+ throw new IllegalArgumentException("Unexpected type: " + typeId);
Review Comment:
may be use switch/case?
##########
modules/core/src/test/java/org/apache/ignite/internal/util/io/IgniteUnsafeDataInputOutputByteOrderTest.java:
##########
@@ -237,4 +249,101 @@ public void testDoubleArray() throws Exception {
assertArrayEquals(arr, in.readDoubleArray(ARR_LEN), 0);
}
+
+ @Test
+ public void testLocalTime() throws IOException {
+ LocalTime val = LocalTime.of(RND.nextInt(24), RND.nextInt(60), RND.nextInt(60), RND.nextInt(10000));
+
+ out.writeLocalTime(val);
+
+ assertEquals(val, in.readLocalTime());
+ }
+
+ @Test
+ public void testLocalDate() throws IOException {
+ LocalDate val = LocalDate.of(RND.nextInt(500) + 1900, RND.nextInt(12) + 1, 1 + RND.nextInt(27));
Review Comment:
```suggestion
LocalDate val = LocalDate.of(RND.nextInt(4000) - 1000, RND.nextInt(12) + 1, 1 + RND.nextInt(27));
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/DefaultValue.java:
##########
@@ -68,9 +92,28 @@ public Type type() {
return type;
}
+ /** Serializes this default value or {@code null}. */
+ public abstract void writeTo(IgniteDataOutput os) throws IOException;
+
+ /** Reads default value or {@code null}. */
+ public static @Nullable DefaultValue readFrom(IgniteDataInput in) throws IOException {
Review Comment:
First, there is a "static" version of `writeTo` somewhere below (hard to understand).
Secondly, from my point of view it might be worth implementing them identically... I understand that this is more like structured programming and not OOP, but it seems in this particular case it will be simpler, shorter and clearer.
```
/** Writes the given default value into output. */
public static void writeTo(@Nullable DefaultValue val, IgniteDataOutput out) throws IOException {
if (val == null) {
out.writeByte(Type.NO_DEFAULT);
} else {
out.writeByte(val.type.typeId);
if (val.type == Type.CONSTANT) {
ConstantValue val0 = (ConstantValue) val;
writeValue(val0.columnType, val0.value, out);
} else {
assert val instanceof FunctionCall;
out.writeUTF(((FunctionCall) val).functionName());
}
}
}
```
wdyt?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/DefaultValue.java:
##########
@@ -51,10 +65,20 @@ public static DefaultValue constant(@Nullable Object value) {
/** Types of the defaults. */
public enum Type {
/** Default is specified as a constant. */
- CONSTANT,
+ CONSTANT(0),
/** Default is specified as a call to a function. */
- FUNCTION_CALL
+ FUNCTION_CALL(1);
+
+ /** Represents absent of default value ({@code null}). */
+ private static final int NO_DEFAULT = -1;
+
+ /** type id used by serialization. */
Review Comment:
```suggestion
/** Type id used by serialization. */
```
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #3627:
URL: https://github.com/apache/ignite-3/pull/3627#discussion_r1571804669
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/DefaultValue.java:
##########
@@ -68,9 +92,28 @@ public Type type() {
return type;
}
+ /** Serializes this default value or {@code null}. */
+ public abstract void writeTo(IgniteDataOutput os) throws IOException;
+
+ /** Reads default value or {@code null}. */
+ public static @Nullable DefaultValue readFrom(IgniteDataInput in) throws IOException {
Review Comment:
I agree. Removed writeTo and moved all logic to static 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #3627:
URL: https://github.com/apache/ignite-3/pull/3627#discussion_r1571900351
##########
modules/core/src/main/java/org/apache/ignite/internal/util/io/IgniteDataOutput.java:
##########
@@ -129,6 +139,86 @@ public interface IgniteDataOutput extends DataOutput {
*/
void writeCharArray(char[] arr) throws IOException;
+ /**
+ * Writes big integer.
+ *
+ * @param val Big integer.
+ * @throws IOException In case of error.
+ */
+ void writeBigInteger(BigInteger val) throws IOException;
+
+ /**
+ * Writes decimal.
+ *
+ * @param val Decimal.
Review Comment:
Done.
##########
modules/core/src/test/java/org/apache/ignite/internal/util/io/IgniteUnsafeDataInputOutputByteOrderTest.java:
##########
@@ -237,4 +249,101 @@ public void testDoubleArray() throws Exception {
assertArrayEquals(arr, in.readDoubleArray(ARR_LEN), 0);
}
+
+ @Test
+ public void testLocalTime() throws IOException {
+ LocalTime val = LocalTime.of(RND.nextInt(24), RND.nextInt(60), RND.nextInt(60), RND.nextInt(10000));
+
+ out.writeLocalTime(val);
+
+ assertEquals(val, in.readLocalTime());
+ }
+
+ @Test
+ public void testLocalDate() throws IOException {
+ LocalDate val = LocalDate.of(RND.nextInt(500) + 1900, RND.nextInt(12) + 1, 1 + RND.nextInt(27));
+
+ out.writeLocalDate(val);
+
+ assertEquals(val, in.readLocalDate());
+ }
+
+ @Test
+ public void testLocalDateTime() throws IOException {
+ LocalTime time = LocalTime.of(RND.nextInt(24), RND.nextInt(60), RND.nextInt(60), RND.nextInt(10000));
+ LocalDate date = LocalDate.of(RND.nextInt(500) + 1900, RND.nextInt(12) + 1, 1 + RND.nextInt(27));
Review Comment:
Fixed, thanks.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #3627:
URL: https://github.com/apache/ignite-3/pull/3627#discussion_r1571899954
##########
modules/core/src/test/java/org/apache/ignite/internal/util/io/IgniteUnsafeDataInputOutputByteOrderTest.java:
##########
@@ -237,4 +249,101 @@ public void testDoubleArray() throws Exception {
assertArrayEquals(arr, in.readDoubleArray(ARR_LEN), 0);
}
+
+ @Test
+ public void testLocalTime() throws IOException {
+ LocalTime val = LocalTime.of(RND.nextInt(24), RND.nextInt(60), RND.nextInt(60), RND.nextInt(10000));
+
+ out.writeLocalTime(val);
+
+ assertEquals(val, in.readLocalTime());
+ }
+
+ @Test
+ public void testLocalDate() throws IOException {
+ LocalDate val = LocalDate.of(RND.nextInt(500) + 1900, RND.nextInt(12) + 1, 1 + RND.nextInt(27));
+
+ out.writeLocalDate(val);
+
+ assertEquals(val, in.readLocalDate());
+ }
+
+ @Test
+ public void testLocalDateTime() throws IOException {
+ LocalTime time = LocalTime.of(RND.nextInt(24), RND.nextInt(60), RND.nextInt(60), RND.nextInt(10000));
+ LocalDate date = LocalDate.of(RND.nextInt(500) + 1900, RND.nextInt(12) + 1, 1 + RND.nextInt(27));
+ LocalDateTime val = LocalDateTime.of(date, time);
+
+ out.writeLocalDateTime(val);
+
+ assertEquals(val, in.readLocalDateTime());
+ }
+
+ @Test
+ public void testInstant() throws IOException {
+ Instant val = Instant.ofEpochMilli(Math.abs(RND.nextLong()));
Review Comment:
Thanks, done.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #3627:
URL: https://github.com/apache/ignite-3/pull/3627#discussion_r1571289616
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/DefaultValue.java:
##########
@@ -68,9 +92,28 @@ public Type type() {
return type;
}
+ /** Serializes this default value or {@code null}. */
+ public abstract void writeTo(IgniteDataOutput os) throws IOException;
+
+ /** Reads default value or {@code null}. */
+ public static @Nullable DefaultValue readFrom(IgniteDataInput in) throws IOException {
Review Comment:
First, the fact that there is a "static" version of `writeTo` somewhere below is difficult to understand.
Secondly, from my point of view it might be worth implementing them identically... I understand that this is more like structured programming and not OOP, but it seems in this particular case it will be simpler, shorter and clearer.
```
/** Writes the given default value into output. */
public static void writeTo(@Nullable DefaultValue val, IgniteDataOutput out) throws IOException {
if (val == null) {
out.writeByte(Type.NO_DEFAULT);
} else {
out.writeByte(val.type.typeId);
if (val.type == Type.CONSTANT) {
ConstantValue val0 = (ConstantValue) val;
writeValue(val0.columnType, val0.value, out);
} else {
assert val instanceof FunctionCall;
out.writeUTF(((FunctionCall) val).functionName());
}
}
}
```
wdyt?
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #3627:
URL: https://github.com/apache/ignite-3/pull/3627#discussion_r1572234538
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/storage/CatalogEntrySerializationTest.java:
##########
@@ -144,6 +174,85 @@ void test(MarshallableEntryType type) {
}
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("values")
+ public void constantDefault(ColumnType columnType, Object value) throws IOException {
+ ConstantValue val = (ConstantValue) DefaultValue.constant(value);
+
+ log.info("{}: {}", columnType, value);
+
+ try (IgniteUnsafeDataOutput os = new IgniteUnsafeDataOutput(128)) {
+ DefaultValue.writeTo(val, os);
+
+ try (IgniteUnsafeDataInput in = new IgniteUnsafeDataInput(os.internalArray())) {
+ DefaultValue actual = DefaultValue.readFrom(in);
+ assertEquals(val, actual);
+ }
+ }
+ }
+
+ private static Stream<Arguments> values() {
+ List<Object> list = new ArrayList<>();
+
+ list.add(null);
+ list.add(RND.nextBoolean());
+
+ list.add((byte) RND.nextInt());
+ list.add((short) RND.nextInt());
+ list.add(RND.nextInt());
+ list.add(RND.nextLong());
+ list.add((float) RND.nextDouble());
+ list.add(RND.nextDouble());
+
+ list.add(BigDecimal.valueOf(RND.nextLong()));
+ list.add(BigDecimal.valueOf(RND.nextLong(), RND.nextInt(100)));
+
+ list.add(BigInteger.valueOf(RND.nextLong()));
+
+ list.add(LocalTime.of(RND.nextInt(24), RND.nextInt(60), RND.nextInt(60), RND.nextInt(100_000)));
+ list.add(LocalDate.of(RND.nextInt(4000) - 1000, RND.nextInt(12) + 1, RND.nextInt(27) + 1));
+ list.add(LocalDateTime.of(
+ LocalDate.of(RND.nextInt(4000) - 1000, RND.nextInt(12) + 1, RND.nextInt(27) + 1),
+ LocalTime.of(RND.nextInt(24), RND.nextInt(60), RND.nextInt(60), RND.nextInt(100_000))
+ ));
+
+ byte[] bytes = new byte[RND.nextInt(1000)];
+ RND.nextBytes(bytes);
+ list.add(Base64.getEncoder().encodeToString(bytes));
+
+ list.add(UUID.randomUUID());
+
+ // TODO Include ignored values to test after https://issues.apache.org/jira/browse/IGNITE-15200
+ // list.add(Duration.of(11, ChronoUnit.HOURS));
+ // list.add(Period.of(5, 4, 3));
+
+ BitSet bitSet = new BitSet();
+ for (int i = 0; i < RND.nextInt(100); i++) {
+ int b = RND.nextInt(1024);
+ bitSet.set(b);
+ }
+ list.add(bitSet);
+
+ return list.stream().map(val -> {
+ NativeType nativeType = NativeTypes.fromObject(val);
+ return Arguments.of(nativeType == null ? ColumnType.NULL : nativeType.spec().asColumnType(), val);
+ });
+ }
+
+ @Test
+ public void functionCallDefault() throws IOException{
Review Comment:
Actually, such tests already exist (for example in `checkAlterColumnEntry`). If you prefer to keep this test separate, I also suggest renaming it with a "test" prefix :thinking:
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/storage/CatalogEntrySerializationTest.java:
##########
@@ -144,6 +174,85 @@ void test(MarshallableEntryType type) {
}
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("values")
+ public void constantDefault(ColumnType columnType, Object value) throws IOException {
Review Comment:
```suggestion
public void testConstantDefaultAllTypes(ColumnType columnType, Object value) throws IOException {
```
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/commands/DefaultValue.java:
##########
@@ -68,9 +92,25 @@ public Type type() {
return type;
}
+ /** Reads default value or {@code null}. */
+ public static @Nullable DefaultValue readFrom(IgniteDataInput in) throws IOException {
Review Comment:
I suggest moving this method below just before `writeTo()`..
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-21435: Sql. Catalog DefaultValue should not depend on Serializable. [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #3627:
URL: https://github.com/apache/ignite-3/pull/3627#discussion_r1572614292
##########
modules/catalog/src/test/java/org/apache/ignite/internal/catalog/storage/CatalogEntrySerializationTest.java:
##########
@@ -144,6 +174,85 @@ void test(MarshallableEntryType type) {
}
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("values")
+ public void constantDefault(ColumnType columnType, Object value) throws IOException {
Review Comment:
Fixed.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org