[Time Conversion] Timestamp to time #482

Devjiu · 2023-05-15T16:43:27Z

Adding timestamp to time conversion. The units is also taken into account.

Resolves: #318

Signed-off-by: Dmitrii Makarenko [email protected]

omniscidb/Tests/QueryBuilderTest.cpp

ienkovich · 2023-05-16T17:06:16Z

omniscidb/QueryBuilder/QueryBuilder.cpp

      return {builder_, expr_->cast(new_type), "", true};
    }
  } else if (expr_->type()->isTimestamp()) {
-    if (new_type->isNumber() || new_type->isDate() || new_type->isTimestamp()) {
+    if (new_type->isInteger() || new_type->isDate() || new_type->isTime() ||


You can use isDateTime here.

ienkovich · 2023-05-16T17:07:32Z

omniscidb/QueryEngine/CastIR.cpp

@@ -108,6 +108,14 @@ llvm::Value* CodeGenerator::codegenCast(llvm::Value* operand_lv,
          operand_type->as<hdk::ir::TimestampType>()->unit(),
          type->nullable());
    }
+    if (operand_type->isTimestamp() && type->isTime()) {
+      const auto operand_unit = (operand_type->isTimestamp())


You know here operand_type is a timestamp.

ienkovich · 2023-05-16T17:12:39Z

omniscidb/QueryEngine/CastIR.cpp

+                                    ? operand_type->as<hdk::ir::TimestampType>()->unit()
+                                    : hdk::ir::TimeUnit::kSecond;
+      // if (operand_unit != type->as<hdk::ir::TimestampType>()->unit()) {
+      return codegenCastTimestampToTime(operand_lv, operand_type, type, type->nullable());


I don't see why nullalbe should be a separate argument if it can be extracted from types. Also, not clear why we use nullable from the target type, not the source.

removed unnecessary argument.

ienkovich · 2023-05-16T17:25:58Z

omniscidb/QueryEngine/CastIR.cpp

+  LOG(ERROR) << "  units from: " << operand_unit << " to: " << target_unit;
+  CHECK(ts_lv->getType()->isIntegerTy(64));
+  const auto target_sec_scale = hdk::ir::unitsPerSecond(target_unit) /
+                                hdk::ir::unitsPerSecond(hdk::ir::TimeUnit::kSecond);


No need to divide by 1.

ienkovich · 2023-05-16T17:27:47Z

omniscidb/QueryEngine/CastIR.cpp

+                                 {{ts_lv,
+                                   cgen_state_->llInt(scale),
+                                   cgen_state_->llInt(target_sec_scale),
+                                   cgen_state_->inlineIntNull(ctx.int64())}});


Null arg is not required here.

ienkovich · 2023-05-16T17:28:06Z

omniscidb/QueryEngine/CastIR.cpp

+                                   {{ts_lv,
+                                     cgen_state_->llInt(scale),
+                                     cgen_state_->llInt(target_sec_scale),
+                                     cgen_state_->inlineIntNull(ctx.int64())}});


Null arg is not required here.

ienkovich · 2023-05-16T17:31:51Z

omniscidb/QueryEngine/CastIR.cpp

+                               {{ts_lv,
+                                 cgen_state_->llInt(scale),
+                                 cgen_state_->llInt(target_sec_scale),
+                                 cgen_state_->inlineIntNull(ctx.int64())}});


I think you need to use operand_type to get a proper null value.

agree, passing source and target null values

ienkovich · 2023-05-16T17:33:35Z

omniscidb/QueryEngine/DateTruncate.cpp

+                                                   const int64_t target_sec_scale,
+                                                   const int64_t null_val) {
+  if (timeval == null_val) {
+    return null_val;


Here we need to return the target null value, not the source one.

agree, done

ienkovich · 2023-05-16T18:20:55Z

omniscidb/Tests/QueryBuilderTest.cpp

+
+    createTable("test_tmstmp",
+                {{"col_bi", ctx().int64()},
+                 {"col_tmstp", ctx().timestamp(hdk::ir::TimeUnit::kSecond)},


Also, try non-nullable types. For nullable types add some NULL values.

made 2 tests, first one is non-nullable, second one is nullable.

Devjiu · 2023-05-31T17:48:31Z

I am not sure, that nulls handled in correct way. Arrow array in case Time32[s] will have int32, canonical type is int64, so in ResultSet will be null int64_t, after conversion to arrow int32 null.

ienkovich

Overall looks good!

I just have some non-functional comments.

The only open question remaining for me is negative timestamps. I see them in tests but don't actually understand how we handle them using unsigned mod. Can you elaborate on this?

ienkovich · 2023-06-06T16:24:44Z

omniscidb/QueryEngine/DateTruncate.cpp

+// scale is 10^{3,6,9}
+inline DEVICE int64_t
+timestamp_to_time_truncation_upscale(const int64_t timeval,
+                                     const int64_t conersion_scale,


conersion -> conversion in multiple places

ienkovich · 2023-06-06T16:28:47Z

omniscidb/QueryEngine/DateTruncate.cpp

+timestamp_to_time_truncation_upscale(const int64_t timeval,
+                                     const int64_t conersion_scale,
+                                     const int64_t target_sec_scale) {
+  return unsigned_mod(timeval * conersion_scale, target_sec_scale * kSecsPerDay);


So what about negative values? Do we support them?

yes, changed the order of calculation. On downscale we at first cut of date part, than cut of unused unit precision. Tests also modified to align with this behavior.

ienkovich · 2023-06-06T16:29:08Z

omniscidb/QueryEngine/CastIR.cpp

+    const auto scale =
+        hdk::ir::unitsPerSecond(target_unit) / hdk::ir::unitsPerSecond(operand_unit);
+
+    // codegenCastBetweenIntTypesOverflowChecks(ts_lv, operand_type, target_type, scale);


Remove comment?

ienkovich · 2023-06-06T16:29:14Z

omniscidb/QueryEngine/CastIR.cpp

+  const auto scale =
+      hdk::ir::unitsPerSecond(operand_unit) / hdk::ir::unitsPerSecond(target_unit);
+
+  // codegenCastBetweenIntTypesOverflowChecks(ts_lv, operand_type, target_type, scale);


Remove comment?

ienkovich · 2023-06-06T16:30:45Z

omniscidb/ResultSet/ResultSet.cpp

-          oss << getStrTimeFromSeconds(boost::get<int64_t>(current_scalar));
+          oss << getStrTStamp(boost::get<int64_t>(current_scalar),
+                              col_type->as<hdk::ir::TimeType>()->unit())
+              << " scalar: " << current_scalar;


I think this function is used to print result sets in PyHDK, scalar values would be really unexpected there.

getStrTStamp print all time types as timestamp, it looks like a debug function. If it's for end user we should rewrite it.

ienkovich · 2023-06-06T16:36:27Z

omniscidb/Tests/QueryBuilderTest.cpp

@@ -519,6 +562,180 @@ TEST_F(QueryBuilderTest, Arithmetics) {
  compare_res_data(res, std::vector<int64_t>({0, NULL_BIGINT}));
 }

+TEST_F(QueryBuilderTest, TmstmpToTime) {


Please use unabbreviated TimestampToTime

Adding timestamp to time conversion. The units is also taken into account. Resolves: #318 Signed-off-by: Dmitrii Makarenko <[email protected]>

Devjiu force-pushed the dmitriim/tmstmp_to_time branch from de68bcd to 510ca18 Compare May 16, 2023 12:58

Devjiu changed the title ~~[Date Conversion] Timestamp to time~~ [Time Conversion] Timestamp to time May 16, 2023

Devjiu force-pushed the dmitriim/tmstmp_to_time branch 4 times, most recently from 586db69 to c83d743 Compare May 16, 2023 15:44

Devjiu commented May 16, 2023

View reviewed changes

omniscidb/Tests/QueryBuilderTest.cpp Show resolved Hide resolved

ienkovich reviewed May 16, 2023

View reviewed changes

Devjiu force-pushed the dmitriim/tmstmp_to_time branch 5 times, most recently from 19e5a39 to 7125f3c Compare May 25, 2023 18:24

Devjiu force-pushed the dmitriim/tmstmp_to_time branch 2 times, most recently from 1e60caa to a587707 Compare May 31, 2023 17:43

Devjiu marked this pull request as ready for review May 31, 2023 17:44

ienkovich suggested changes Jun 6, 2023

View reviewed changes

Devjiu force-pushed the dmitriim/tmstmp_to_time branch 2 times, most recently from 0ae014d to d14f208 Compare June 9, 2023 14:04

[Time Conversion] Timestamp to time

48d8b98

Adding timestamp to time conversion. The units is also taken into account. Resolves: #318 Signed-off-by: Dmitrii Makarenko <[email protected]>

Devjiu force-pushed the dmitriim/tmstmp_to_time branch from d14f208 to 48d8b98 Compare June 14, 2023 17:58

ienkovich approved these changes Jun 14, 2023

View reviewed changes

kurapov-peter merged commit c89a4ed into main Jun 15, 2023

kurapov-peter deleted the dmitriim/tmstmp_to_time branch June 15, 2023 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Time Conversion] Timestamp to time #482

[Time Conversion] Timestamp to time #482

Devjiu commented May 15, 2023

ienkovich May 16, 2023

ienkovich May 16, 2023

Devjiu May 31, 2023

ienkovich May 16, 2023

Devjiu May 31, 2023

ienkovich May 16, 2023

Devjiu May 31, 2023

ienkovich May 16, 2023

Devjiu May 31, 2023

ienkovich May 16, 2023

Devjiu May 31, 2023

ienkovich May 16, 2023

Devjiu May 31, 2023

ienkovich May 16, 2023

Devjiu May 31, 2023

ienkovich May 16, 2023

Devjiu May 31, 2023

Devjiu commented May 31, 2023

ienkovich left a comment

ienkovich Jun 6, 2023

Devjiu Jun 9, 2023

ienkovich Jun 6, 2023

Devjiu Jun 9, 2023

ienkovich Jun 6, 2023

Devjiu Jun 9, 2023

ienkovich Jun 6, 2023

Devjiu Jun 9, 2023

ienkovich Jun 6, 2023

Devjiu Jun 9, 2023

ienkovich Jun 6, 2023

Devjiu Jun 9, 2023

[Time Conversion] Timestamp to time #482

[Time Conversion] Timestamp to time #482

Conversation

Devjiu commented May 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Devjiu commented May 31, 2023

ienkovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment