ibessonov commented on code in PR #6931:
URL: https://github.com/apache/ignite-3/pull/6931#discussion_r2555629670
##########
modules/api/src/main/java/org/apache/ignite/table/mapper/MapperBuilder.java:
##########
@@ -114,13 +114,7 @@ private static <O> Class<O> ensureValidPojo(Class<O> type)
{
throw new IllegalArgumentException("Unsupported class. Interfaces
are not supported: " + type.getName());
}
- try {
- type.getDeclaredConstructor();
-
- return type;
- } catch (NoSuchMethodException e) {
- throw new IllegalArgumentException("Class must have default
constructor: " + type.getName());
- }
+ return type;
Review Comment:
Now there's no validation here, although the method is called
`ensureValidPojo`. So, two questions:
- Should we get rid of mentioning `pojo` in the name of the method?
- Should we get back the more sophisticated validation?
My opinion in both cases - I think we should
##########
modules/java-records-tests/src/testFixtures/java/org/apache/ignite/internal/schema/marshaller/Records.java:
##########
@@ -0,0 +1,429 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.schema.marshaller;
+
+import java.util.Objects;
+import org.apache.ignite.catalog.annotations.Column;
+import org.apache.ignite.internal.schema.SchemaDescriptor;
+import org.apache.ignite.internal.type.NativeTypes;
+
+/**
+ * Declaration variants fixture. Each nested class contains record and its
class alternative declarations.
+ */
+class Records {
+ private Records() {}
+
+ static final String SQL_PATTERN = "CREATE TABLE {} (key INT PRIMARY KEY,
val VARCHAR, val2 VARCHAR)";
+
+ static final SchemaDescriptor schema = new SchemaDescriptor(1,
+ new org.apache.ignite.internal.schema.Column[]{
+ new org.apache.ignite.internal.schema.Column("KEY",
NativeTypes.INT32, false)
+ },
+ new org.apache.ignite.internal.schema.Column[]{
+ new org.apache.ignite.internal.schema.Column("VAL",
NativeTypes.STRING, true),
+ new org.apache.ignite.internal.schema.Column("VAL2",
NativeTypes.STRING, true)
+ });
+
+ static class ComponentsExact {
+ record RecordK(@Column("key") Integer id) { }
+ record RecordV(@Column("val") String val) { }
+ record Record(@Column("key") Integer id, @Column("val") String val) { }
+
+ record ExplicitCanonical(@Column("key") Integer id, @Column("val")
String val) {
+ ExplicitCanonical(@Column("key") Integer id, @Column("val") String
val) {
+ this.id = id;
+ this.val = val;
+ if (id == null && val == null) {
+ throw new IllegalArgumentException("null args");
+ }
+ }
+ }
+
+ record ExplicitCanonicalV(@Column("val") String val) {
+ ExplicitCanonicalV(@Column("val") String val) {
+ this.val = val;
+ if (val == null) {
+ throw new IllegalArgumentException("null args");
+ }
+ }
+ }
+
+ public record ExplicitCompact(@Column("key") Integer id,
@Column("val") String val) {
+ public ExplicitCompact {
+ if (id == null && val == null) {
+ throw new IllegalArgumentException("null args");
+ }
+ }
+ }
+
+ public record ExplicitCompactV(@Column("val") String val) {
+ public ExplicitCompactV {
+ if (val == null) {
+ throw new IllegalArgumentException("null args");
+ }
+ }
+ }
+
+ record ExplicitMultiple(@Column("key") Integer id, @Column("val")
String val) {
+ ExplicitMultiple(Integer id) {
+ this(id, "");
+ }
+
+ ExplicitMultiple(String val) {
+ this(-1, val);
+ }
+ }
Review Comment:
Please add a no-arguments constructor too. I believe that some of your tests
will fail after that
##########
modules/marshaller-common/src/main/java/org/apache/ignite/internal/marshaller/AnnotatedConstructor.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.marshaller;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.lang.util.IgniteNameUtils.parseIdentifier;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Parameter;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.ignite.catalog.annotations.Column;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.NotNull;
+
+class AnnotatedConstructor implements Creator.Annotated {
+ private final MethodHandle mhFixedArity;
+ private final List<String> argsColumnNames;
+
+ AnnotatedConstructor(Constructor<?> ctor) {
+ assert ctor.getParameterCount() > 0;
+ this.argsColumnNames = argsNames(ctor);
+ this.mhFixedArity = unreflect(ctor);
+ }
+
+ @Override
+ public Object createFrom(FieldAccessor[] accessors, MarshallerReader
reader) {
+ try {
+ NamedArguments args = readArgs(accessors, reader);
+ return mhFixedArity.invokeWithArguments(args.values);
+ } catch (MarshallerException e) {
+ throw e;
+ } catch (Throwable e) {
+ throw new IllegalArgumentException("Failed to instantiate class",
e);
+ }
+ }
+
+ private static List<String> argsNames(Constructor<?> ctor) {
+ Parameter[] creatorParams = ctor.getParameters();
+
+ if (Arrays.stream(creatorParams).anyMatch(p ->
p.getAnnotation(Column.class) == null)) {
Review Comment:
```suggestion
if (Arrays.stream(creatorParams).anyMatch(p ->
!p.isAnnotationPresent(Column.class))) {
```
##########
modules/marshaller-common/src/main/java/org/apache/ignite/internal/marshaller/Creator.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.marshaller;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.util.Arrays;
+import org.jetbrains.annotations.Nullable;
+
+class Creator<T> {
+ final Class<T> clazz;
+ private final FieldAccessor[] accessors;
+ private @Nullable Annotated defaultConstructor;
+ private @Nullable Annotated creatorConstructor;
+
+ Creator(Class<T> clazz, FieldAccessor[] accessors) {
+ this.clazz = clazz;
+ this.accessors = accessors;
+ findCreators(clazz);
+ }
+
+ @FunctionalInterface
+ interface Annotated {
+ Object createFrom(FieldAccessor[] accessors, MarshallerReader reader);
+ }
Review Comment:
Any specific reason why it's called `Annotated`? Maybe there's a better name
that explains what this interface means
##########
modules/marshaller-common/src/main/java/org/apache/ignite/internal/marshaller/AnnotatedConstructor.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.marshaller;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.lang.util.IgniteNameUtils.parseIdentifier;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Parameter;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.ignite.catalog.annotations.Column;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.NotNull;
+
+class AnnotatedConstructor implements Creator.Annotated {
Review Comment:
I suggest renaming this class, the name is misleading
##########
modules/marshaller-common/src/main/java/org/apache/ignite/internal/marshaller/AnnotatedFieldsWithDefaultConstructor.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.marshaller;
+
+import static java.lang.invoke.MethodType.methodType;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Constructor;
+import org.apache.ignite.lang.MarshallerException;
+
+class AnnotatedFieldsWithDefaultConstructor implements Creator.Annotated {
+ private final MethodHandle mhNoArgs;
+
+ AnnotatedFieldsWithDefaultConstructor(Constructor<?> defaultCtor) {
+ assert defaultCtor.getParameterCount() == 0;
+ this.mhNoArgs = unreflect(defaultCtor);
+ }
+
+ @Override
+ public Object createFrom(FieldAccessor[] accessors, MarshallerReader
reader) {
+ try {
+ Object instance = mhNoArgs.invokeExact();
+
+ for (int fldIdx = 0; fldIdx < accessors.length; fldIdx++) {
+ accessors[fldIdx].read(reader, instance);
+ }
Review Comment:
Can you please comment on why you're expecting that the order in `accessors`
will match the order from `reader`? Maybe there are some invariants that you
forgot to document?
##########
modules/marshaller-common/src/main/java/org/apache/ignite/internal/marshaller/Marshaller.java:
##########
@@ -283,13 +284,14 @@ private static class PojoMarshaller extends Marshaller {
/** {@inheritDoc} */
@Override
public Object readObject(MarshallerReader reader, Object target)
throws MarshallerException {
- Object obj = target == null ? factory.create() : target;
-
- for (int fldIdx = 0; fldIdx < fieldAccessors.length; fldIdx++) {
- fieldAccessors[fldIdx].read(reader, obj);
+ if (target != null) {
+ for (int fldIdx = 0; fldIdx < fieldAccessors.length; fldIdx++)
{
+ fieldAccessors[fldIdx].read(reader, target);
+ }
+ return target;
Review Comment:
Is this a copy of the code in `AnnotatedFieldsWithDefaultConstructor`?
If that's the case (it appears to be) then there's certainly something wrong
with the design here, would it be possible to improve the architecture of this
code?
##########
modules/marshaller-common/src/main/java/org/apache/ignite/internal/marshaller/Creator.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.marshaller;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Method;
+import java.util.Arrays;
+import org.jetbrains.annotations.Nullable;
+
+class Creator<T> {
+ final Class<T> clazz;
+ private final FieldAccessor[] accessors;
+ private @Nullable Annotated defaultConstructor;
+ private @Nullable Annotated creatorConstructor;
+
+ Creator(Class<T> clazz, FieldAccessor[] accessors) {
+ this.clazz = clazz;
+ this.accessors = accessors;
+ findCreators(clazz);
+ }
+
+ @FunctionalInterface
+ interface Annotated {
+ Object createFrom(FieldAccessor[] accessors, MarshallerReader reader);
+ }
+
+ Object createFrom(MarshallerReader reader) {
+ if (creatorConstructor != null) {
+ return creatorConstructor.createFrom(accessors, reader);
+ }
+ if (defaultConstructor != null) {
+ return defaultConstructor.createFrom(accessors, reader);
+ }
+
+ throw new IllegalArgumentException("Could not find default (no-args)
or canonical (record) constructor for " + clazz.getName());
+ }
+
+ private void findCreators(Class<T> clazz) {
+ Constructor<?> defaultCtor = null;
+ Constructor<?> creatorCtor = null;
+
+ Constructor<?>[] ctors = clazz.getDeclaredConstructors();
+ for (Constructor<?> ctor : ctors) {
+ if (ctor.isSynthetic()) {
+ continue;
+ }
+ if (ctor.getParameterCount() == 0) {
+ defaultCtor = ctor;
+ continue;
+ }
+
+ if (isCanonicalConstructor(clazz, ctor)) {
+ creatorCtor = ctor;
+ }
+ }
Review Comment:
It makes no sense to look for canonical constructor if class is not a
`Record` class, which can be checked in advance. Please make this code simpler
and more rigorous. Should be as simple as this:
```
if (isRecord(clazz)) {
return findCanonicalConstructor(clazz);
} else {
return fineDefaultConstructor(clazz);
}
```
No need to iterate over all declared constructors.
##########
modules/marshaller-common/src/main/java/org/apache/ignite/internal/marshaller/AnnotatedConstructor.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.marshaller;
+
+import static java.util.stream.Collectors.toList;
+import static org.apache.ignite.lang.util.IgniteNameUtils.parseIdentifier;
+
+import java.lang.invoke.MethodHandle;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Parameter;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.ignite.catalog.annotations.Column;
+import org.apache.ignite.lang.MarshallerException;
+import org.jetbrains.annotations.NotNull;
+
+class AnnotatedConstructor implements Creator.Annotated {
+ private final MethodHandle mhFixedArity;
+ private final List<String> argsColumnNames;
+
+ AnnotatedConstructor(Constructor<?> ctor) {
+ assert ctor.getParameterCount() > 0;
+ this.argsColumnNames = argsNames(ctor);
+ this.mhFixedArity = unreflect(ctor);
+ }
+
+ @Override
+ public Object createFrom(FieldAccessor[] accessors, MarshallerReader
reader) {
+ try {
+ NamedArguments args = readArgs(accessors, reader);
+ return mhFixedArity.invokeWithArguments(args.values);
+ } catch (MarshallerException e) {
+ throw e;
+ } catch (Throwable e) {
+ throw new IllegalArgumentException("Failed to instantiate class",
e);
+ }
+ }
+
+ private static List<String> argsNames(Constructor<?> ctor) {
+ Parameter[] creatorParams = ctor.getParameters();
+
+ if (Arrays.stream(creatorParams).anyMatch(p ->
p.getAnnotation(Column.class) == null)) {
Review Comment:
Can also be `! allMatch( isAnnotationPresent )`, it's up to you to decide
what looks better
--
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]
