You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "xtern (via GitHub)" <gi...@apache.org> on 2024/07/24 12:31:01 UTC
Re: [PR] IGNITE-22283 Sql. Prepare to remove NUMBER and BITMASK native types [ignite-3]
xtern commented on code in PR #4114:
URL: https://github.com/apache/ignite-3/pull/4114#discussion_r1689644368
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/registry/UpgradingRowAdapter.java:
##########
@@ -331,15 +331,7 @@ public BigDecimal decimalValue(int colIdx, int scale) {
/** {@inheritDoc} */
@Override
public BigInteger numberValue(int colIdx) throws InvalidTypeException {
- int mappedId = mapColumn(colIdx);
-
- Column column = mappedId < 0 ? mapper.mappedColumn(colIdx) : row.schema().column(mappedId);
-
- if (NativeTypeSpec.NUMBER != column.type().spec()) {
- throw new SchemaException("Type conversion is not supported yet.");
- }
-
- return mappedId < 0 ? (BigInteger) column.defaultValue() : row.numberValue(mappedId);
+ throw new UnsupportedOperationException("Number is not supported");
Review Comment:
looks like numberValue/bitmaskValue in `InternalTuple` should also be marked as deprecated?
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/table/ItPublicApiColocationTest.java:
##########
@@ -168,9 +158,7 @@ private static Stream<Arguments> twoColumnsParameters() {
for (NativeTypeSpec t0 : NativeTypeSpec.values()) {
for (NativeTypeSpec t1 : NativeTypeSpec.values()) {
- if (!EXCLUDED_TYPES.contains(t0) && !EXCLUDED_TYPES.contains(t1)) {
- args.add(Arguments.of(t0, t1));
- }
+ args.add(Arguments.of(t0, t1));
Review Comment:
This is not a valid change until we remove these types from the NativeTypeSpec :sunglasses:
##########
modules/schema/src/test/java/org/apache/ignite/internal/schema/AbstractSchemaConverterTest.java:
##########
@@ -67,15 +66,13 @@ public class AbstractSchemaConverterTest extends BaseIgniteAbstractTest {
tmp.put(NativeTypeSpec.DATETIME, Arrays.asList(null, LocalDateTime.MIN, LocalDateTime.MAX, LocalDateTime.now()));
tmp.put(NativeTypeSpec.TIMESTAMP, Arrays.asList(null, Instant.MIN, Instant.MAX, Instant.EPOCH, Instant.now()));
tmp.put(NativeTypeSpec.UUID, Arrays.asList(null, UUID.randomUUID()));
- tmp.put(NativeTypeSpec.BITMASK, Arrays.asList(null, fromBinString(""), fromBinString("1"),
- fromBinString("10101010101010101010101")));
tmp.put(NativeTypeSpec.STRING, Arrays.asList(null, "", UUID.randomUUID().toString()));
tmp.put(NativeTypeSpec.BYTES, Arrays.asList(null, ArrayUtils.BYTE_EMPTY_ARRAY,
UUID.randomUUID().toString().getBytes(StandardCharsets.UTF_8)));
- tmp.put(NativeTypeSpec.NUMBER, Arrays.asList(null, BigInteger.ONE, BigInteger.ZERO,
- new BigInteger("10000000000000000000000000000000000000")));
var missedTypes = new HashSet<>(Arrays.asList(NativeTypeSpec.values()));
+ missedTypes.remove(NativeTypeSpec.NUMBER);
Review Comment:
:thinking: from my point of view all similar changes in tests must be covered with TODO "issue to remove native types"
##########
modules/schema/src/test/java/org/apache/ignite/internal/schema/marshaller/RecordMarshallerTest.java:
##########
@@ -219,24 +218,7 @@ public void classWithWrongFieldType(MarshallerFactory factory) {
assertThat(
ex.getMessage(),
- containsString("Column's type mismatch [column=LONGCOL, expectedType=BITSET, actualType=class java.lang.Long]"));
- }
-
- @ParameterizedTest
- @MethodSource("marshallerFactoryProvider")
- public void classWithIncorrectBitmaskSize(MarshallerFactory factory) {
- SchemaDescriptor schema = new SchemaDescriptor(
- schemaVersion.incrementAndGet(),
- new Column[]{ new Column("key".toUpperCase(), INT32, false) },
- new Column[]{ new Column("bitmaskCol".toUpperCase(), NativeTypes.bitmaskOf(9), true) }
- );
-
- RecordMarshaller<TestBitmaskObject> marshaller = factory.create(schema, TestBitmaskObject.class);
-
- TestBitmaskObject rec = new TestBitmaskObject(1, IgniteTestUtils.randomBitSet(rnd, 42));
Review Comment:
`IgniteUtils.randomBitSet` is not used anymore, so from my point of view it should be removed
##########
modules/schema/src/test/java/org/apache/ignite/internal/schema/catalog/CatalogToSchemaDescriptorConverterTest.java:
##########
@@ -58,8 +56,13 @@ public class CatalogToSchemaDescriptorConverterTest extends AbstractSchemaConver
private static final int TEST_SCALE = 5;
+ private static Stream<NativeTypeSpec> nativeTypeSpecs() {
+ return Stream.of(NativeTypeSpec.values())
+ .filter(t -> t != NativeTypeSpec.NUMBER && t != NativeTypeSpec.BITMASK);
+ }
+
@ParameterizedTest
- @EnumSource(NativeTypeSpec.class)
+ @MethodSource("nativeTypeSpecs")
Review Comment:
from my point of view all similar changes in tests must be covered with TODO "issue to remove native types"
##########
modules/schema/src/testFixtures/java/org/apache/ignite/internal/schema/SchemaTestUtils.java:
##########
@@ -207,8 +190,10 @@ public static NativeType specToType(NativeTypeSpec spec) {
public static void ensureAllTypesChecked(Stream<Column> allColumns) {
Set<NativeTypeSpec> testedTypes = allColumns.map(c -> c.type().spec())
.collect(Collectors.toSet());
+ Set<NativeTypeSpec> unsupported = Set.of(NativeTypeSpec.BITMASK, NativeTypeSpec.NUMBER);
Review Comment:
this also should be marked with TODO :thinking:
##########
modules/storage-page-memory/src/test/java/org/apache/ignite/internal/storage/pagememory/index/InlineUtilsTest.java:
##########
@@ -53,6 +54,7 @@ public class InlineUtilsTest extends BaseIgniteAbstractTest {
@Test
void testInlineSizeForNativeType() {
EnumSet<NativeTypeSpec> nativeTypeSpecs = EnumSet.allOf(NativeTypeSpec.class);
+ nativeTypeSpecs.removeAll(Set.of(NativeTypeSpec.BITMASK, NativeTypeSpec.NUMBER));
Review Comment:
and this
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/util/ProjectedTupleTest.java:
##########
@@ -84,6 +85,9 @@ void allTypesAreCovered() {
EnumSet<NativeTypeSpec> allTypes = EnumSet.allOf(NativeTypeSpec.class);
+ Set<NativeTypeSpec> unsupported = Set.of(NativeTypeSpec.BITMASK, NativeTypeSpec.NUMBER);
Review Comment:
this also better to be marked with TODO :sunglasses:
##########
modules/table/src/test/java/org/apache/ignite/internal/table/KeyValueViewOperationsSimpleSchemaTest.java:
##########
@@ -490,7 +489,12 @@ public void putGetAllTypes() {
);
// Validate all types are tested.
- assertEquals(Set.of(NativeTypeSpec.values()),
+ Set<NativeTypeSpec> nativeTypes = EnumSet.allOf(NativeTypeSpec.class);
+
+ Set<NativeTypeSpec> unsupported = Set.of(NativeTypeSpec.BITMASK, NativeTypeSpec.NUMBER);
Review Comment:
and this
##########
modules/system-view/src/test/java/org/apache/ignite/internal/systemview/SystemViewManagerTest.java:
##########
@@ -163,6 +164,11 @@ public void registerAllColumnTypes(NativeTypeSpec typeSpec) {
assertTrue(viewMgr.completeRegistration().isDone());
}
+ private static Stream<NativeTypeSpec> nativeTypeSpecs() {
Review Comment:
and this
##########
modules/table/src/integrationTest/java/org/apache/ignite/internal/table/ItColocationTest.java:
##########
@@ -373,9 +367,13 @@ private static Object generateValueByType(int i, NativeTypeSpec type) {
private static Stream<Arguments> twoColumnsParameters() {
List<Arguments> args = new ArrayList<>();
+ Set<NativeTypeSpec> unsupported = Set.of(NativeTypeSpec.BITMASK, NativeTypeSpec.NUMBER);
for (NativeTypeSpec t0 : NativeTypeSpec.values()) {
for (NativeTypeSpec t1 : NativeTypeSpec.values()) {
+ if (unsupported.contains(t0) || unsupported.contains(t1)) {
Review Comment:
and this
##########
modules/table/src/test/java/org/apache/ignite/internal/table/KeyValueViewOperationsTest.java:
##########
@@ -668,7 +666,11 @@ private KeyValueViewImpl<TestKeyObject, TestObjectWithAllTypes> kvView() {
// Validate all types are tested.
Set<NativeTypeSpec> testedTypes = Arrays.stream(valCols).map(c -> c.type().spec())
.collect(Collectors.toSet());
+
+ Set<NativeTypeSpec> unsupported = Set.of(NativeTypeSpec.BITMASK, NativeTypeSpec.NUMBER);
+
Set<NativeTypeSpec> missedTypes = Arrays.stream(NativeTypeSpec.values())
+ .filter(t -> !unsupported.contains(t))
Review Comment:
and this
##########
modules/table/src/test/java/org/apache/ignite/internal/table/MutableRowTupleAdapterTest.java:
##########
@@ -470,11 +468,17 @@ public void testFullSchemaHasAllTypes() {
.map(NativeTypeSpec::asColumnType)
.collect(Collectors.toSet());
+ Set<ColumnType> unsupported = Set.of(ColumnType.BITMASK, ColumnType.NUMBER);
+
for (ColumnType columnType : ColumnType.values()) {
if (columnType == ColumnType.NULL) {
continue;
}
+ if (unsupported.contains(columnType)) {
Review Comment:
and this
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItAlterTableDdlTest.java:
##########
@@ -152,11 +150,6 @@ public void testNullableColumn() {
public void testDropAndAddColumnsAllTypes() {
List<NativeType> allTypes = SchemaTestUtils.ALL_TYPES;
- Set<NativeTypeSpec> unsupportedTypes = Set.of(
- // TODO https://issues.apache.org/jira/browse/IGNITE-18431
- NativeTypeSpec.BITMASK
Review Comment:
what about other places where [IGNITE-18431](https://issues.apache.org/jira/browse/IGNITE-18431) is mentioned?
Should we mention that all such places should be cleared in the `follower` ticket?
##########
modules/table/src/test/java/org/apache/ignite/internal/table/RecordViewOperationsTest.java:
##########
@@ -435,7 +433,11 @@ private RecordViewImpl<TestObjectWithAllTypes> recordView() {
// Validate all types are tested.
Set<NativeTypeSpec> testedTypes = Arrays.stream(valCols).map(c -> c.type().spec())
.collect(Collectors.toSet());
+
+ Set<NativeTypeSpec> unsupported = Set.of(NativeTypeSpec.BITMASK, NativeTypeSpec.NUMBER);
+
Set<NativeTypeSpec> missedTypes = Arrays.stream(NativeTypeSpec.values())
+ .filter(t -> !unsupported.contains(t))
Review Comment:
and this
--
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