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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
