Hi Timo, Thanks for the suggestion!
I updated the FLIP to use `isPrimitive()` and `VariantTypeException`. Regarding `JSON()` and `PARSE_JSON()`, we should keep the return type of the `JSON()` method as it is, and use the `PARSE_JSON()` method to convert a JSON string to a variant type. `JSON`, `JSON_OBJECT`, `JSON_ARRAY` are a set of methods to build a JSON string, while `PARSE_JSON` converts a JSON string to a variant data type, which can work well together and should not cause confusion. You are right that casting the variant to the STRUCTURED and ROW types is useful. It should be included in the FLIP, while we may not support it in the first version. The FLIP is updated. Best, Xuannan On Mon, May 5, 2025 at 9:54 PM Timo Walther <twal...@apache.org> wrote: > > Hi Xuannan, > > thank you for updating the FLIP and addressing my comments. The FLIP is > in a very good shape now, +1 for voting. I just found some last > remaining items that would be good to be addressed in the FLIP. But not > blocker for the voting process: > > > isScalar() > > I thought about the naming again. I would rather vote for > `isPrimitive()`. Our ScalarFunctions are also able to take and return > arrays and maps. An `isPrimitive()` should avoid this confusion. > > > VariantUnexpectedTypeException > > Rather call it VariantTypeException? Shorter is better. Otherwise a > UnexpectedVariantTypeException would read better. But a general > VariantTypeException should be good enough. > > > I cannot figure out how it can work with the current JSON_OBJECT and > JSON_ARRAY functions > > We left the return type of JSON() open to future work. JSON() can only > be used directly as parameters for values in JSON_OBJECT and JSON_ARRAY, > because we didn't have a JSON type so far. So JSON() can return a > VARIANT type that is directly understood by JSON_OBJECT and JSON_ARRAY > functions. > I'm fine with PARSE_JSON as well, but we should double check the > semantics with JSON() to avoid any kind of confusion. > > > Therefore, instead of going for a non-trivial temporary solution > > that will be removed later, we are better off justbumping the > > Calcite version, which is inevitable. > > Great to hear that we aim to update Calcite. Happy to support reviewing > PRs that bump Calcite versions. I'm sure Sergey can help here as well. > We should try to perform this update in stages to avoid introducing > regressions. > > > Only a scalar value Variant is allowed to be cast to the > > corresponding type > > Can you elaborate on this? It could be quite useful for the Table API > integration if we could supporting casting to STRUCTURED types and thus > POJOs in the future. This doesn't have to be supported in version 1 but > could be useful. For rows and structured types, we support nested > casting when calling functions. > > Looking forward to this feature! > > Regards, > Timo > > > On 03.05.25 01:32, Xuannan Su wrote: > > Hi everyone, > > > > Thank you for all the comments! Please let me know if you have further > > comments. If there are no further comments, I'd > > like to close the discussion and start the voting in a few days. > > > > Best, > > Xuannan > > > > > > On Thu, May 1, 2025 at 20:21 Xuannan Su <suxuanna...@gmail.com> wrote: > > > >> Hi Aihua, > >> > >> We planning to support Variant shredding soon after we support the > >> basis variant encoding as stated in the Future Work section of the > >> FLIP. > >> > >> Best, > >> Xuannan > >> > >> On Thu, May 1, 2025 at 1:43 PM Aihua Xu <aihu...@apache.org> wrote: > >>> > >>> Hi Xuannan, > >>> > >>> Are we planning to support Variant shredding in the future or just basic > >> variant encoding for Flink? > >>> > >>> Thanks, > >>> Aihua > >> > > >
