How to make rescaling utility functions more general?

[wuxueyang96]

As mentioned in #6051 (comment) .

After #5801, when rescaling utility functions are implemented, it's better to return an error message instead of throwing an exception, and it will probably introduce a field called errorMessage. But if we introduce some APIs like folly::Expected or something like that, it might be more clear. Should we introduce these APIs?

If a rescaling function returns folly::Expected, it should be folly::Expected<TOutput, ERROR> which TOutput stands int64_t or int128_t and ERROR could be a string-type or an enum-type value. If we use a string-type value as ERROR, it might contain a lot of copying so an enum-type may be better.

Should we add a new enum class for the rescaling function's error reason? And What should be contained in this enum class?

/cc @mbasmanova @jinchengchenghh @rui-mo

[mbasmanova]

CC: @xiaoxmeng @laithsakka @bikramSingh91 @pedroerp @Yuhta

[mbasmanova]

Some references:

• folly::Expected: https://github.com/facebook/folly/blob/main/folly/Expected.h
• absl::StatusOr: https://abseil.io/docs/cpp/guides/status

[mbasmanova]

@wuxueyang96

If we use a string-type value as ERROR, it might contain a lot of copying so an enum-type may be better.

Would you test this to confirm whether we indeed incur lots of copying for the error message strings? This data point would help direct the discussion.

[Yuhta]

I would vote for something like rocksdb::Status: https://github.com/facebook/rocksdb/blob/main/include/rocksdb/status.h
In hot path it is lightweight, but allow to pass in more details if needed.

[jinchengchenghh]

arrow::Result https://github.com/apache/arrow/blob/main/cpp/src/arrow/result.h#L97

[jinchengchenghh]

Can folly ERROR be std::string_view to avoid possible string copy?

[Yuhta]

Life cycle will be tricky to manage if we pass a string view around as a result

[bikramSingh91]

I think there are 2 different mechanism to vote for/discuss here:

1. A Status object and what to include in its contents?
   Any generic Status class that has a status code/ENUM (for quick comparison/checks of error types) and more members for details that can be std::move'ed to keep them lightweight while passing them around in return statements.
   We can also just write our own Status class since they are quite simple and incrementally add members based on our need.

As previously suggested, for this we can refer to any of the available implementations like absl::Status , rocksdb::Status , arrow::Result , impala:Status, kudu:Status

2. A mechanism that can bind together a result and error ( absl::StatusOr , folly::Expected , std::expected , where we can fix the second template to be the Status Object we defined above) VS Only returning a Status object and return the result as a referenced input param.
   This is a choice that aligns more with how you want to structure the code and less of a performance thing like Point 1. I would personally prefer something like the former but I am not sure if templating would add any complexity.

BTW, is the plan to start with lower level APIs like cast conversion utils and eventually bubble them up to higher levels like expression eval's main loop?

[Yuhta]

I think for now we only use it for casting/rescale to address performance issue caused by exceptions. For higher level exceptions are fine as long as we are not doing async stuff.

[jinchengchenghh]

Do we have conclusion?
I vote for implementing Velox own Status and Expected<T>.

[mbasmanova]

@jinchengchenghh Would you like to draft a PR? It might be easier to decide once we see what the code looks like.

[jinchengchenghh]

I will take a very long vocation, can you help draft a PR? @duanmeng

[duanmeng]

@jinchengchenghh Sure, let me take a look. Enjoy your vacation and congratulations in advance.

[pedroerp]

I also vote we create a lightweight status class instead of folly::Expected (or similar) as it gives us more flexibility to evolve it and add the members we need. Although we are only targeting the rescaling path for now, I suspect this will quickly end up being used in other parts of the codebase.

Happy to help iterate on a PR is anyone would like to take a stab at it.

[zhztheplayer]

I am just curious about the refactor scope. If we wanted to replace all exception-throwing code in Velox to status-return (do we?), will one have to catch exceptions and handle returned statuses at the same time when calling Velox's API? Since exceptions still be thrown from std library or possibly from 3rd libraries.

Just wanted to make sure we won't create more complexity to users here.

[majetideepak]

I think we should simply add it to the rescaling path for now. We can convert Status to Velox Error outside.
This will be helpful for integrating Arrow Parquet Writer code in Velox as well.
Any thoughts on using Arrow's Status https://github.com/apache/arrow/blob/main/cpp/src/arrow/status.h?
Edit: I mean copying Arrow's Status.

[Yuhta]

We don't want to add arrow as a dependency on core lib so it's better to create our own

[majetideepak]

Yes, I meant to copy it, not as a dependency.

[FelixYBW]

So currently only parquet writer is copied from arrow, right? What's the long team plan for Velox Parquet writer? Will we re-implement it or use arrow as it is?

I don't think copying the code from arrow is good idea. We will eventually customize code but just leave the "arrow" name there without hardcoded dependency.

[pedroerp]

The idea is to slowly blend that code into our codebase. Most of that is already done (#6193)

[pedroerp]

I start looking at this as part of vendoring the Parquet writer code (#6193). I'll send out a draft soon based on the Status/Result classes Arrow has today, which are pretty handy.

Cc: @majetideepak @mbasmanova

[pedroerp]

FYI, I just merged today a first version of a Status class for Velox. It's heavily based on Arrow's:

#8084

When I'm back from holiday's break I'll submit a proposal for an Result/Expect class. Seems like the one in folly is pretty functional and get be easily used with our Status class.