Add SparkSql function to_pretty_string #10359

jinchengchenghh · 2024-07-01T10:06:35Z

Spark implement: https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToPrettyString.scala

This is an internal spark function.

netlify · 2024-07-01T10:06:51Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`a4ae7d4`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66c6aa81ff2b0e0008b69c2f

jinchengchenghh · 2024-07-02T03:13:20Z

@rui-mo @PHILO-HE Can you help review this PR? Thanks!

velox/functions/sparksql/ToPrettyString.h

PHILO-HE · 2024-07-03T03:08:52Z

velox/docs/functions/spark/string.rst

+        SELECT toprettystring(null);  -- "NULL"
+        SELECT toprettystring(cast('abcd' as binary));  -- "[61 62 63 64 65 66]"
+
+.. spark:function:: toprettystring(x, timeZone) -> varchar


I think timeZone can be removed and let the above doc for toprettystring(x) cover timestamp type. TimeZone is actually read from Velox config. And like other time/date function, Gluten will not directly pass it to native function.

Thanks! I change the implement but forget the document.

PHILO-HE · 2024-07-03T03:24:46Z

velox/functions/sparksql/Register.cpp

+      {prefix + "toprettystring"});
+  registerFunction<ToPrettyStringFunction, Varchar, float>(
+      {prefix + "toprettystring"});
+  registerFunction<ToPrettyStringFunction, Varchar, double>(


Better to use a helper function for these numeric types. E.g., add registerUnaryNumeric in RegistrationHelpers.h, then use it here.

velox/functions/sparksql/ToPrettyString.h

PHILO-HE · 2024-07-03T03:49:57Z

velox/functions/sparksql/ToPrettyString.h

+    if constexpr (std::is_same_v<TInput, StringView>) {
+      if (inputType_->isVarchar()) {
+        result = input;
+      } else if (inputType_->isVarbinary()) {


The existing code for cast cannot be re-used?

Yes, as the comments indicate https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToPrettyString.scala#L30

PHILO-HE · 2024-07-03T03:53:20Z

velox/functions/sparksql/ToPrettyString.h

+  template <typename TInput>
+  void callNullable(out_type<Varchar>& result, const TInput* a) {
+    if (a) {
+      call(result, *a);


As call method co-exists, the non-null input will directly go into call method and will not go here. Maybe, we can only keep callNullable method with the impl. for call covered.

+1. We can merge the implementation of call into callNullable.

PHILO-HE · 2024-07-03T03:54:58Z

velox/functions/sparksql/ToPrettyString.h

+  template <typename TInput>
+  void callNullable(out_type<Varchar>& result, const TInput* a) {
+    if (a) {
+      call(result, *a);


velox/functions/sparksql/ToPrettyString.h

mbasmanova · 2024-07-03T21:08:19Z

velox/functions/sparksql/ToPrettyString.h

+        input, scale_, maxRowSize_, result.data());
+    if (view.isInline()) {
+      result.setNoCopy(view);
+      result.resize(0);


This doesn't look right. Why are we resizing to 0?

It is right. The result is inline, so we don't need to allocate from buffer.

Here we write some bytes to result.data() indeed, so it looks not right to resize it to 0. Alternatively, we could write data to an inlined buffer and set it to result.

char inlined[StringView::kInlineSize];

mbasmanova · 2024-07-03T21:08:56Z

velox/functions/sparksql/ToPrettyString.h

+      result.setNoCopy(view);
+      result.resize(0);
+    } else {
+      result.resize(view.size());


I think there is not need for if-else block. You can just do this unconditionally.

This is for decimal, min long decimal value -1'000'000'000'000'000'000 + 1 with . is not inline

velox/functions/sparksql/ToPrettyString.h

rui-mo

Thanks. Added several comments.

rui-mo · 2024-07-05T05:51:47Z

velox/docs/functions/spark/string.rst

@@ -283,6 +283,20 @@ Unless specified otherwise, all functions return NULL if at least one of the arg
        SELECT substring_index('aaaaa', 'aa', 5); -- "aaaaa"
        SELECT substring_index('aaaaa', 'aa', -5); -- "aaaaa"

+.. spark:function:: toprettystring(x) -> varchar


Function name suggestion: to_pretty_string

This name is used in Spark plan, so I use this name.

Perhaps follow Velox's style for the function name and add a mapping in compute engine.

rui-mo · 2024-07-05T06:00:48Z

velox/docs/functions/spark/string.rst

+
+    - It prints binary values (either from column or struct field) using the hex format. ::
+
+        SELECT toprettystring('spark');  -- "spark"


The queries fail in Spark as internal function cannot be called by user. Shall we remove from documentation if their tests are covered?

rui-mo · 2024-07-05T06:01:09Z

velox/docs/functions/spark/string.rst

+        SELECT toprettystring('spark');  -- "spark"
+        SELECT toprettystring(12);  -- "12"
+        SELECT toprettystring(null);  -- "NULL"
+        SELECT toprettystring(cast('abcd' as binary));  -- "[61 62 63 64 65 66]"


Is the result [61 62 63 64]?

rui-mo · 2024-07-05T06:01:50Z

velox/docs/functions/spark/string.rst

+        SELECT toprettystring(12);  -- "12"
+        SELECT toprettystring(null);  -- "NULL"
+        SELECT toprettystring(cast('abcd' as binary));  -- "[61 62 63 64 65 66]"
+        SELECT toprettystring('2000-01-01 12:21:56');  -- "2000-01-01 04:21:56"


Perhaps document how timezone is handled.

rui-mo · 2024-07-05T06:07:35Z

velox/expression/CastExpr-inl.h

@@ -51,6 +51,9 @@ inline std::exception_ptr makeBadCastException(
      makeErrorMessage(input, row, resultType, errorDetails),
      false));
 }
+} // namespace
+
+namespace detail {


We may need to extract convertToStringView out in a header to use it elsewhere.

APIs in 'detail' namespaces are not supposed to be used outside of the header/cpp files.
#10241 (comment)

velox/functions/sparksql/ToPrettyString.h

rui-mo · 2024-07-05T07:25:31Z

velox/functions/sparksql/ToPrettyString.h

+        input, scale_, maxRowSize_, result.data());
+    if (view.isInline()) {
+      result.setNoCopy(view);
+      result.resize(0);


Here we write some bytes to result.data() indeed, so it looks not right to resize it to 0. Alternatively, we could write data to an inlined buffer and set it to result.

char inlined[StringView::kInlineSize];

velox/functions/sparksql/ToPrettyString.h

jinchengchenghh · 2024-07-05T07:55:01Z

setNoCopy will copy the data to StringView itself when it is inline.
https://github.com/facebookincubator/velox/blob/main/velox/vector/FlatVector.cpp#L137
So we don't need to reserve the data in String allocators @rui-mo

rui-mo · 2024-07-05T08:04:02Z

@jinchengchenghh I notice some bytes are written to result.data(), but resizing it to 0 does not clear the data. Therefore, result has some data but its size is zero, which looks not correct.
https://github.com/facebookincubator/velox/blob/main/velox/functions/UDFOutputString.h#L45-L51

rui-mo · 2024-07-18T05:49:00Z

velox/docs/functions/spark/string.rst

@@ -283,6 +283,20 @@ Unless specified otherwise, all functions return NULL if at least one of the arg
        SELECT substring_index('aaaaa', 'aa', 5); -- "aaaaa"
        SELECT substring_index('aaaaa', 'aa', -5); -- "aaaaa"

+.. spark:function:: toprettystring(x) -> varchar


Perhaps follow Velox's style for the function name and add a mapping in compute engine.

rui-mo · 2024-07-18T05:54:53Z

velox/docs/functions/spark/string.rst

@@ -283,6 +283,16 @@ Unless specified otherwise, all functions return NULL if at least one of the arg
        SELECT substring_index('aaaaa', 'aa', 5); -- "aaaaa"
        SELECT substring_index('aaaaa', 'aa', -5); -- "aaaaa"

+.. spark:function:: toprettystring(x) -> varchar
+
+    Returns pretty string for all scalar type. It has several differences with casting value to string:


Document suggestion:

Returns pretty string for the input value ``x``. All scalar types are supported. There are several differences with the behavior of casting ``x`` as string.

Also notice Spark describes it to support all kinds of values, and are we choosing to support scalar types only?

Yes, I don't support complex datatype now.

rui-mo · 2024-07-18T05:57:10Z

velox/docs/functions/spark/string.rst

+
+    - It prints binary values (either from column or struct field) using the hex format.
+
+    Considering the time zone if config ``session_timezone`` is set. ::


Moved to L288.

rui-mo · 2024-07-18T05:59:27Z

velox/functions/sparksql/Register.cpp

@@ -165,6 +166,29 @@ inline void registerArrayMinMaxFunctions(const std::string& prefix) {
  registerArrayMinMaxFunctions<Timestamp>(prefix);
  registerArrayMinMaxFunctions<Date>(prefix);
 }
+
+void registerToPrettyStringFunctions(const std::string& prefix) {
+  registerUnaryIntegralWithTReturn<ToPrettyStringFunction, Varchar>(


nit: declare a const variable to avoid repeating "toprettystring".

velox/functions/sparksql/Register.cpp

rui-mo · 2024-07-18T06:07:52Z

velox/functions/sparksql/ToPrettyString.h

+      }
+      if constexpr (std::is_same_v<TInput, int32_t>) {
+        if (inputType_->isDate()) {
+          auto output = DATE()->toString(*input);


This conversion could also throw exception as it calls DateType::toIso8601. We may need to handle the exception correctly.

Yes, thanks for your catch. I will change the return type to Status.
Spark will also throw exception when it cannot be valid Date, the LocalDate.ofEpochDay(days) may throw exception. https://github.com/apache/spark/blob/branch-3.5/sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/DateFormatter.scala#L70

rui-mo · 2024-07-18T06:09:06Z

velox/functions/sparksql/ToPrettyString.h

+          util::Converter<TypeKind::VARCHAR, void, util::SparkCastPolicy>::
+              tryCast(*input);
+      if (castResult.hasError()) {
+        result.setNoCopy(detail::kNull);


This indicates for invaild input null is returned. Is that the case in Spark? Perhaps also document and test it.

rui-mo · 2024-07-18T06:10:51Z

velox/functions/sparksql/ToPrettyString.h

+/// toprettystring(x) -> varchar
+/// Returns pretty string for int8, int16, int32, int64, bool, Date, Varchar. It
+/// has one difference with casting value to string:
+/// - It prints null values (either from column or struct field) as "NULL".


nit: Date -> date, Varchar -> varchar to align with others.

/// It has one difference with casting value to string:
/// - It prints null values (either from column or struct field) as "NULL".

Comment suggestion: The difference with casting input value as string is that "NULL" is returned for null inputs.

either from column or struct field

It seems complex type is not supported. Do we need this description?

rui-mo · 2024-07-18T06:12:58Z

velox/functions/sparksql/ToPrettyString.h

+/// Returns pretty string for int8, int16, int32, int64, bool, Date, Varchar. It
+/// has one difference with casting value to string:
+/// - It prints null values (either from column or struct field) as "NULL".
+


nit: remove this empty line.

rui-mo · 2024-07-18T06:16:18Z

velox/functions/sparksql/ToPrettyString.h

+};
+
+/// Returns pretty string for Varbinary. It has several differences with casting
+/// value to string:


nit: Varbinary -> varbinary
There are several differences with cast(varbinary as string).

rui-mo · 2024-07-18T06:21:21Z

velox/functions/sparksql/ToPrettyString.h

+
+/// Returns pretty string for Varbinary. It has several differences with casting
+/// value to string:
+/// - It prints null values (either from column or struct field) as "NULL".


nit: - > 1), perhaps remove either from column or struct field as varbinary could not have struct field.

rui-mo · 2024-07-18T06:21:43Z

velox/functions/sparksql/ToPrettyString.h

+/// Returns pretty string for Varbinary. It has several differences with casting
+/// value to string:
+/// - It prints null values (either from column or struct field) as "NULL".
+/// - It prints binary values (either from column or struct field) using the hex


rui-mo · 2024-07-18T06:35:55Z

velox/functions/sparksql/ToPrettyString.h

+/// - It prints null values (either from column or struct field) as "NULL".
+/// - It prints binary values (either from column or struct field) using the hex
+/// format. Returns a pretty string of the byte array which prints each byte as
+/// a hex digit and add spaces between them. For example, [1A C0].


Returns a pretty string of the byte array which prints each byte as a hex digit and add spaces between them. For example, [1A C0].

Comment suggestion:

/// The pretty string is composed of the hex digits of bytes and spaces between
/// them. E.g., the result of to_pretty_string("abc") is "[31 32 33]".

rui-mo · 2024-07-18T06:38:18Z

velox/functions/sparksql/ToPrettyString.h

+  TypePtr inputType_;
+};
+
+/// Returns pretty string for Varbinary. It has several differences with casting


It seems the binary format is controlled by a config SQLConf.BINARY_OUTPUT_STYLE, and shall we document that only hex format is supported?

https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToPrettyString.scala#L52

In the Spark3.5 version, it use default hex format version. https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ToPrettyString.scala#L52

rui-mo · 2024-07-18T06:42:14Z

velox/functions/sparksql/ToPrettyString.h

+      char* pos = startPosition;
+      *pos++ = '[';
+      for (auto i = 0; i < input->size(); i++) {
+        auto formated = fmt::format("{:X}", input->data()[i]);


nit: formated -> formatted

jinchengchenghh · 2024-08-20T03:21:21Z

Address all the comments.

rui-mo · 2024-08-21T09:16:35Z

velox/docs/functions/spark/string.rst

@@ -342,6 +342,16 @@ String Functions
        SELECT substring_index('aaaaa', 'aa', 5); -- "aaaaa"
        SELECT substring_index('aaaaa', 'aa', -5); -- "aaaaa"

+.. spark:function:: to_pretty_string(x) -> varchar
+
+    Returns pretty string for all scalar type. Considering the time zone if config ``session_timezone`` is set. 


Document suggestion:

Returns pretty string for ``x``. All scalar types are supported. Adjusts the timestamp input to the given time zone if set through ``session_timezone`` config.

rui-mo · 2024-08-21T09:18:40Z

velox/docs/functions/spark/string.rst

+
+    Returns pretty string for all scalar type. Considering the time zone if config ``session_timezone`` is set. 
+
+    It has several differences with casting value to string:


Document suggestion:

The result is different from that of casting ``x`` as string in the following aspects.

rui-mo · 2024-08-21T09:24:16Z

velox/docs/functions/spark/string.rst

+
+    It has several differences with casting value to string:
+
+    - It prints null values as "NULL".


Document suggestion:

It prints null input as "NULL" rather than producing null output.

rui-mo · 2024-08-21T09:35:28Z

velox/functions/sparksql/ToPrettyString.h

+/// to_pretty_string(x) -> varchar
+/// Returns pretty string for int8, int16, int32, int64, bool, Date, Varchar. It
+/// has one difference with casting value to string:
+/// 1) It prints null values as "NULL".


It prints null input as "NULL" rather than producing null output.

rui-mo · 2024-08-21T10:04:27Z

velox/functions/sparksql/ToPrettyString.h

+      for (auto i = 0; i < input->size(); i++) {
+        auto formatted = fmt::format("{:X}", input->data()[i]);
+        *pos++ = formatted.data()[0];
+        *pos++ = formatted.data()[1];


Perhaps we can use 'snprintf' to write to 'pos' directly and avoid creating intermediate string.

std::snprintf(pos, 2, "%02X", static_cast(input->data()[i]));

rui-mo · 2024-08-21T10:15:46Z

velox/docs/functions/spark/string.rst

+
+    - It prints null values as "NULL".
+
+    - It prints binary values using the hex format.


It prints binary value as hex string representation rather than UTF-8.

rui-mo · 2024-08-22T07:04:04Z

velox/functions/sparksql/ToPrettyString.h

+            Timestamp::tsToStringView(inputValue, options_, result.data());
+        result.resize(stringView.size());
+      } catch (const std::exception& e) {
+        return Status::Invalid(e.what());


Try-catch is used for date and timestamp types. Wondering if it is preferred to make these APIs return status.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 1, 2024

rui-mo reviewed Jul 3, 2024

View reviewed changes

velox/functions/sparksql/ToPrettyString.h Outdated Show resolved Hide resolved

PHILO-HE reviewed Jul 3, 2024

View reviewed changes

mbasmanova reviewed Jul 3, 2024

View reviewed changes

rui-mo reviewed Jul 5, 2024

View reviewed changes

rui-mo reviewed Jul 18, 2024

View reviewed changes

jinchengchenghh force-pushed the toprettystring branch from d808d2b to ff52ca8 Compare August 19, 2024 03:42

jinchengchenghh changed the title ~~Add SparkSql function toprettystring~~ Add SparkSql function to_pretty_string Aug 19, 2024

rui-mo reviewed Aug 21, 2024

View reviewed changes

jinchengchenghh force-pushed the toprettystring branch from 2026b1a to 77fd4a0 Compare August 22, 2024 02:05

rui-mo reviewed Aug 22, 2024

View reviewed changes

jinchengchenghh added 6 commits August 22, 2024 09:41

support toprettystring

e72e037

address comments

6dc0690

minor

0d83a63

minor

0a21e8b

address comments

77fd4a0

suport unknown type

a4ae7d4


		- It prints binary values (either from column or struct field) using the hex format. ::

		SELECT toprettystring('spark'); -- "spark"


		- It prints binary values (either from column or struct field) using the hex format.

		Considering the time zone if config ``session_timezone`` is set. ::


		Returns pretty string for all scalar type. Considering the time zone if config ``session_timezone`` is set.

		It has several differences with casting value to string:


		It has several differences with casting value to string:

		- It prints null values as "NULL".


		- It prints null values as "NULL".

		- It prints binary values using the hex format.

Add SparkSql function to_pretty_string #10359

Are you sure you want to change the base?

Add SparkSql function to_pretty_string #10359

Conversation

jinchengchenghh commented Jul 1, 2024

netlify bot commented Jul 1, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

jinchengchenghh commented Jul 2, 2024

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

PHILO-HE Jul 3, 2024 • edited Loading

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

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

jinchengchenghh commented Jul 5, 2024

rui-mo commented Jul 5, 2024 • edited Loading

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

jinchengchenghh commented Aug 20, 2024

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

netlify bot commented Jul 1, 2024 •

edited

Loading

PHILO-HE Jul 3, 2024 •

edited

Loading

rui-mo commented Jul 5, 2024 •

edited

Loading