mihaibudiu commented on code in PR #4843:
URL: https://github.com/apache/calcite/pull/4843#discussion_r2989768905


##########
arrow/src/test/java/org/apache/calcite/adapter/arrow/ArrowAdapterDataTypesTest.java:
##########
@@ -222,4 +222,76 @@ static void initializeArrowState(@TempDir Path 
sharedTempDir)
         .returns(result)
         .explainContains(plan);
   }
+
+  /** Test case for
+   * <a 
href="https://issues.apache.org/jira/browse/CALCITE-6646";>[CALCITE-6646]
+   * Arrow adapter should support Timestamp data type</a>. */
+  @Test void testTimestampSecProject() {
+    String sql = "select \"timestampSecField\" from arrowdatatype";
+    String plan = "PLAN=ArrowToEnumerableConverter\n"
+        + "  ArrowProject(timestampSecField=[$12])\n"
+        + "    ArrowTableScan(table=[[ARROW, ARROWDATATYPE]], fields=[[0, 1, 
2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]])\n\n";
+    String result = "timestampSecField=2024-01-01 00:00:00\n"
+        + "timestampSecField=2024-01-02 00:00:00\n";
+    CalciteAssert.that()
+        .with(arrow)
+        .query(sql)
+        .limit(2)
+        .returns(result)
+        .explainContains(plan);
+  }
+
+  /** Test case for
+   * <a 
href="https://issues.apache.org/jira/browse/CALCITE-6646";>[CALCITE-6646]
+   * Arrow adapter should support Timestamp data type</a>. */
+  @Test void testTimestampMilliProject() {
+    String sql = "select \"timestampMilliField\" from arrowdatatype";
+    String plan = "PLAN=ArrowToEnumerableConverter\n"
+        + "  ArrowProject(timestampMilliField=[$13])\n"
+        + "    ArrowTableScan(table=[[ARROW, ARROWDATATYPE]], fields=[[0, 1, 
2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]])\n\n";
+    String result = "timestampMilliField=2024-01-01 00:00:00.000\n"
+        + "timestampMilliField=2024-01-02 00:00:00.000\n";
+    CalciteAssert.that()
+        .with(arrow)
+        .query(sql)
+        .limit(2)
+        .returns(result)
+        .explainContains(plan);
+  }
+
+  /** Test case for
+   * <a 
href="https://issues.apache.org/jira/browse/CALCITE-6646";>[CALCITE-6646]
+   * Arrow adapter should support Timestamp data type</a>. */
+  @Test void testTimestampMicroProject() {

Review Comment:
   it looks to me that the conversion from Arrow to Calcite is actually lossy, 
since Calcite only supports milliseconds precision in its enumerable runtime. 
   
   You should have a test which exhibits this behavior.
   
   This behavior should somewhere be documented too.



##########
arrow/src/test/java/org/apache/calcite/adapter/arrow/ArrowDataTest.java:
##########
@@ -430,4 +458,48 @@ private void timeField(FieldVector fieldVector, int 
rowCount) {
     }
     fieldVector.setValueCount(rowCount);
   }
+
+  private void timestampSecField(FieldVector fieldVector, int rowCount) {
+    // Base epoch: 2024-01-01 00:00:00 UTC = 1704067200 seconds
+    TimeStampSecVector tsVector = (TimeStampSecVector) fieldVector;
+    tsVector.setInitialCapacity(rowCount);
+    tsVector.allocateNew();
+    for (int i = 0; i < rowCount; i++) {
+      tsVector.set(i, 1704067200L + i * 86400L);

Review Comment:
   I don't know what these numbers mean, please compute them using some helper 
functions (see DateTimeUtils in Avatica).



##########
arrow/src/main/java/org/apache/calcite/adapter/arrow/AbstractArrowEnumerator.java:
##########
@@ -66,16 +69,51 @@ protected void loadNextArrowBatch() {
 
   @Override public Object current() {
     if (fields.size() == 1) {
-      return this.valueVectors.get(0).getObject(currRowIndex);
+      return getValue(this.valueVectors.get(0), currRowIndex);
     }
     Object[] current = new Object[valueVectors.size()];
     for (int i = 0; i < valueVectors.size(); i++) {
       ValueVector vector = this.valueVectors.get(i);
-      current[i] = vector.getObject(currRowIndex);
+      current[i] = getValue(vector, currRowIndex);
     }
     return current;
   }
 
+  /** Extracts a value from a vector at the given index.
+   *
+   * <p>For {@link TimeStampVector}, converts the raw value to
+   * milliseconds since epoch, which is Calcite's internal representation

Review Comment:
   I am not sure what you mean by "Calcite's internal representation for 
timestamp". There are multiple representations for timestamp constants. In the 
compiler they are RexLiteral values, in the Enumerable runtime they are Java 
long values. But other runtimes may use other representations. 



##########
arrow/src/test/java/org/apache/calcite/adapter/arrow/ArrowDataTest.java:
##########
@@ -430,4 +458,48 @@ private void timeField(FieldVector fieldVector, int 
rowCount) {
     }
     fieldVector.setValueCount(rowCount);
   }
+
+  private void timestampSecField(FieldVector fieldVector, int rowCount) {
+    // Base epoch: 2024-01-01 00:00:00 UTC = 1704067200 seconds
+    TimeStampSecVector tsVector = (TimeStampSecVector) fieldVector;
+    tsVector.setInitialCapacity(rowCount);
+    tsVector.allocateNew();
+    for (int i = 0; i < rowCount; i++) {
+      tsVector.set(i, 1704067200L + i * 86400L);

Review Comment:
   These constants are opaque, can they be computed by a helper function?
   There are lots of helper functions to manipulate timestamps in Calcite and 
Avatica.



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