ibessonov commented on code in PR #2424:
URL: https://github.com/apache/ignite-3/pull/2424#discussion_r1293190792


##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexManager.java:
##########
@@ -521,20 +521,34 @@ private static class TableRowToIndexKeyConverter {
             this.indexedColumns = indexedColumns;
         }
 
-        public BinaryTuple convert(BinaryRow binaryRow) {
+        @Override
+        public BinaryTuple extractColumnsFromKeyOnlyRow(BinaryRow keyOnlyRow) {
+            return 
converter(keyOnlyRow).extractColumnsFromKeyOnlyRow(keyOnlyRow);
+        }
+
+        @Override
+        public BinaryTuple extractColumns(BinaryRow row) {
+            return converter(row).extractColumns(row);
+        }
+
+        private ColumnsExtractor converter(BinaryRow row) {
+            int schemaVersion = row.schemaVersion();
+
             VersionedConverter converter = this.converter;
 
-            if (converter.version != binaryRow.schemaVersion()) {
+            if (converter.version != schemaVersion) {
                 synchronized (mutex) {
-                    if (converter.version != binaryRow.schemaVersion()) {
-                        converter = createConverter(binaryRow.schemaVersion());
+                    converter = this.converter;
+
+                    if (converter.version != schemaVersion) {

Review Comment:
   What do you think about re-implementing this "cache" in the future?
   Small LRU cache may fit the task much better, having only one cached element 
is too optimistic.



##########
modules/schema/src/main/java/org/apache/ignite/internal/util/ObjectFactory.java:
##########
@@ -35,17 +36,25 @@ public class ObjectFactory<T> implements Factory<T> {
      * @throws IllegalArgumentException If no default constructor found.
      */
     public ObjectFactory(Class<T> clazz) {
-        try {
-            cnstr = clazz.getDeclaredConstructor();
-            cnstr.setAccessible(true);
-        } catch (NoSuchMethodException e) {
-            throw new IllegalArgumentException("Class has no default 
constructor: class=" + clazz.getName(), e);
+        if (clazz == Void.class) {
+            cnstr = null;

Review Comment:
   But is it really necessary? I mean, is it documented anywhere?
   Maybe null value always means "no" value and we introduce a bug here?



-- 
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]

Reply via email to