Add to_iso8601(date) Presto function

[svm1]

[netlify[bot]]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	43e3b6ca4db335ee51ebed1e2d76114eff5bfdaf
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/660d919bd406530008f38e87

[majetideepak]

@svm1 I see the following output in Presto for date and timestamp inputs.

presto> select to_ISO8601(cast('1970-01-01' as DATE));
   _col0    
------------
 1970-01-01 
(1 row)

presto> select to_ISO8601(cast('1970-01-01' as timestamp));
             _col0             
-------------------------------
 1970-01-01T00:00:00.000+05:30 
(1 row)

[svm1]

@majetideepak Thanks for pointing that out, I recently made the same observation myself. Corrected behavior to comply with Presto Java.

[svm1]

@majetideepak @aditi-pandit please take a look, changes have been finalized.

[aditi-pandit]

@svm1 : Please can you fix format-check failures.

[svm1]

@aditi-pandit Done.

[majetideepak]

@mbasmanova can you take a look at this PR? Thanks.

[svm1]

@mbasmanova Thanks for the review.

I was told to break up the all-encompassing PR into separate PRs for each function signature, for ease of review, as each set of changes was becoming fairly lengthy. Here are the other two - https://github.com/facebookincubator/velox/pull/8551, https://github.com/facebookincubator/velox/pull/8550. The idea was that they could be merged sequentially, which each new PR further expanding the functionality of to_iso8601() to support the other types.

[mbasmanova]

I was told to break up the all-encompassing PR into separate PRs for each function signature, for ease of review, as each set of changes was becoming fairly lengthy.

Got it. Thank you for clarifying. In the future, please, share this context upfront to help reviewer understand why the PR has some limitations and realize that you'll be asking for more reviews shortly. Code review is labor intensive time consuming activity. Reducing back-and-forth by communicating proactively helps. Thanks.

[mbasmanova]

Implement Presto function to_iso8601() for Date type input, with tests.

This sentence in the PR description is redundant. The title already says that you are adding to_iso8601 function for date inputs. Since every single change must come with tests, saying "with tests" is redundant.

[svm1]

@mbasmanova Sorry for the confusion - I think I better understand your point now about making toIso8601() a static function. Though I did want to mention, there is an existing value-based conversion function in the DateType class -toString(int32 days). Also noticed similar value functions in the IntervalDayTimeType class, another logical type. I was wondering if even this should be present here in the logical type definition, or if it should be made a static function elsewhere, perhaps in TimestampConversion.h. As in, should we be relying on an instance of the class for any value conversion operations in the first place?

[svm1]

@mbasmanova would appreciate your thoughts on the above:

Sorry for the confusion - I think I better understand your point now about making toIso8601() a static function. Though I did want to mention, there is an existing value-based conversion function in the DateType class - toString(int32 days). Also noticed similar value functions in the IntervalDayTimeType class, another logical type. I was wondering if even this should be present here in the logical type definition, or if it should be made a static function elsewhere, perhaps in TimestampConversion.h. As in, should we be relying on an instance of the class for any value conversion operations in the first place?

[mbasmanova]

Also noticed similar value functions in the IntervalDayTimeType class, another logical type. I was wondering if even this should be present here in the logical type definition, or if it should be made a static function elsewhere, perhaps in TimestampConversion.h. As in, should we be relying on an instance of the class for any value conversion operations in the first place?

I haven't looked into these, but I expect these should be static methods, not instance methods.

[svm1]

@mbasmanova Yes, that's my point - the existing value conversion methods for the Date type, in Type.h, are not static methods but instead are instance methods (toString(), toDays()). That's why I was asking if these should also be modified, if you wanted my new toIso8601() method to be static.

[mbasmanova]

I was asking if these should also be modified

I see. It would be nice to update these as well, but in a separate PR. Thanks.

[svm1]

@mbasmanova I've made all the changes - created toIso8601() as a static method and overloaded toString() to take in TimestampToStringOptions as an argument, so it can be called by the former with the necessary flags for ISO formatting. This mitigates the issue of relying on and modifying toString() for the sake of the Presto function implementation.

Please take a look when you get a chance!

[svm1]

Rebased with main again, hitting a new fuzzer failure. Opened issue - https://github.com/facebookincubator/velox/issues/9192

[svm1]

@mbasmanova CI clean now.

[facebook-github-bot]

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@mbasmanova merged this pull request in facebookincubator/velox@e4f74d3bfc219cbf399ca54e9d0bdd3f32fe9d6d.

[conbench-facebook[bot]]

Conbench analyzed the 1 benchmark run on commit e4f74d3b.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.