[GitHub] [ignite-3] rpuch commented on a diff in pull request #2233: IGNITE-19693 getAll does not preserve order and does not return nulls for missing keys

["rpuch (via GitHub)" <gi...@apache.org>]

rpuch commented on code in PR #2233:
URL: https://github.com/apache/ignite-3/pull/2233#discussion_r1241713092


##########
modules/api/src/main/java/org/apache/ignite/table/RecordView.java:
##########
@@ -55,18 +56,20 @@ public interface RecordView<R> extends DataStreamerTarget<R> {
      * @param keyRecs Records with key columns set. The records cannot be {@code null}.
      * @return Records with all columns filled from the table. The order of collection elements is
      *     guaranteed to be the same as the order of {@code keyRecs}. If a record does not exist, the
-     *     element at the corresponding index of the resulting collection is null.
+     *     element at the corresponding index of the resulting collection is {@code null}.
      */
-    Collection<R> getAll(@Nullable Transaction tx, @NotNull Collection<R> keyRecs);
+    List<R> getAll(@Nullable Transaction tx, Collection<R> keyRecs);
 
     /**
      * Asynchronously gets records from a table.
      *
      * @param tx      Transaction or {@code null} to auto-commit.
      * @param keyRecs Records with the key columns set. The records cannot be {@code null}.
-     * @return Future that represents the pending completion of the operation.
+     * @return Future that will return records with all columns filled from the table. The order of collection elements is
+     *      guaranteed to be the same as the order of {@code keyRecs}. If a record does not exist, the
+     *      element at the corresponding index of the resulting collection is {@code null}.
      */
-    @NotNull CompletableFuture<Collection<R>> getAllAsync(@Nullable Transaction tx, @NotNull Collection<R> keyRecs);
+    CompletableFuture<List<R>> getAllAsync(@Nullable Transaction tx, Collection<R> keyRecs);

Review Comment:
   Same question about the type change



##########
modules/api/src/main/java/org/apache/ignite/table/RecordView.java:
##########
@@ -55,18 +56,20 @@ public interface RecordView<R> extends DataStreamerTarget<R> {
      * @param keyRecs Records with key columns set. The records cannot be {@code null}.
      * @return Records with all columns filled from the table. The order of collection elements is
      *     guaranteed to be the same as the order of {@code keyRecs}. If a record does not exist, the
-     *     element at the corresponding index of the resulting collection is null.
+     *     element at the corresponding index of the resulting collection is {@code null}.
      */
-    Collection<R> getAll(@Nullable Transaction tx, @NotNull Collection<R> keyRecs);
+    List<R> getAll(@Nullable Transaction tx, Collection<R> keyRecs);

Review Comment:
   Wny is type change needed here? The whole API seems to consistently use `Collection`. Even if we return `Collection`, we may guarantee the order internally by actually returning a `List` or something else, if we want.



##########
modules/client/src/main/java/org/apache/ignite/internal/client/table/ClientRecordBinaryView.java:
##########
@@ -76,27 +78,26 @@ public Tuple get(@Nullable Transaction tx, @NotNull Tuple keyRec) {
                 ClientTupleSerializer.getPartitionAwarenessProvider(tx, keyRec));
     }
 
-    /** {@inheritDoc} */
     @Override
-    public Collection<Tuple> getAll(@Nullable Transaction tx, @NotNull Collection<Tuple> keyRecs) {
+    public List<Tuple> getAll(@Nullable Transaction tx, Collection<Tuple> keyRecs) {

Review Comment:
   List?



##########
modules/client/src/main/java/org/apache/ignite/internal/client/table/ClientRecordView.java:
##########
@@ -76,27 +78,26 @@ public R get(@Nullable Transaction tx, @NotNull R keyRec) {
                 ClientTupleSerializer.getPartitionAwarenessProvider(tx, ser.mapper(), keyRec));
     }
 
-    /** {@inheritDoc} */
     @Override
-    public Collection<R> getAll(@Nullable Transaction tx, @NotNull Collection<R> keyRecs) {
+    public List<R> getAll(@Nullable Transaction tx, Collection<R> keyRecs) {

Review Comment:
   List?



##########
modules/client/src/main/java/org/apache/ignite/internal/client/table/ClientKeyValueView.java:
##########
@@ -464,11 +464,12 @@ private HashMap<K, V> readGetAllResponse(ClientSchema schema, ClientMessageUnpac
 
         try {
             for (int i = 0; i < cnt; i++) {
-                in.unpackBoolean(); // TODO: Optimize (IGNITE-16022).
-
-                var tupleReader = new BinaryTupleReader(schema.columns().length, in.readBinaryUnsafe());
-                var reader = new ClientMarshallerReader(tupleReader);
-                res.put((K) keyMarsh.readObject(reader, null), (V) valMarsh.readObject(reader, null));
+                // TODO: Optimize (IGNITE-16022).
+                if (in.unpackBoolean()) {

Review Comment:
   Does this boolean mean that the following value is not null?



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