Skip to content

[Coral-Trino] Avoid accidental translation of Trino from_unixtime SQL call#464

Closed
findinpath wants to merge 1 commit intolinkedin:masterfrom
findinpath:findinpath/from_unixtime_fix
Closed

[Coral-Trino] Avoid accidental translation of Trino from_unixtime SQL call#464
findinpath wants to merge 1 commit intolinkedin:masterfrom
findinpath:findinpath/from_unixtime_fix

Conversation

@findinpath
Copy link
Copy Markdown
Contributor

@findinpath findinpath commented Oct 15, 2023

What changes are proposed in this pull request, and why are they necessary?

Avoid the accidental translation of the Trino from_unixtime introduced through #426

Context #459 (comment)

Fixes #459

How was this patch tested?

Regular unit tests

@wmoustafa
Copy link
Copy Markdown
Contributor

I think you would want to implement an operator in coral-trino similar to TrinoElementAtFunction and have the transformer convert to that one as opposed to creating a Hive function (operator). Operators currently part of "Hive Function Registry" will end up in Coral common as they capture Coral IR, and not a language specific set of operators.

@findinpath
Copy link
Copy Markdown
Contributor Author

@wmoustafa

I created an alternative PR #465 to follow-up on your request

think you would want to implement an operator in coral-trino similar to TrinoElementAtFunction

However, I failed in getting it to work exactly because the function was not mentioned in StaticHiveFunctionRegistry.

StaticHiveFunctionRegistry contains already Trino (not Hive) specific content:

createAddUserDefinedFunction("from_unixtime_nanos", explicit(SqlTypeName.TIMESTAMP), NUMERIC);
createAddUserDefinedFunction("$canonicalize_hive_timezone_id", explicit(SqlTypeName.VARCHAR), STRING);

I believe that, in the current state of the coral library, my current PR is the right approach to fix the from_unixtime regresssion reported on #459.

// Date Functions
createAddUserDefinedFunction("from_unixtime", FunctionReturnTypes.STRING,
family(ImmutableList.of(SqlTypeFamily.NUMERIC, SqlTypeFamily.STRING), optionalOrd(1)));
createAddUserDefinedFunction("trino_from_unixtime", explicit(SqlTypeName.TIMESTAMP),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to create an operator (e.g., TimestampFromUnixtime) and have its unparse method return to_unixtimestamp and add it to the HiveFunctionRegistry instead of sticking to
the standard way of creating operators through createAddUserDefinedFunction? I think this can save us from having to create TrinoFromUnixtimeOperatorTransformer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried adding the operator TimestampFromUnixtime and register it in StaticHiveFunctionRegistry, but still failed to get the tests 🟢 .

The not so good thing about using an operator is that we'd need to define it in coral-hive, so that we can make use of it in StaticHiveFunctionRegistry . This is definitely not the right place for a Trino specific operator.

I still think that, in the current circumstances, using the transformer TimestampFromUnixtimeTransformer is a viable option.

@findinpath findinpath force-pushed the findinpath/from_unixtime_fix branch 2 times, most recently from 58bc0ee to 01e78f4 Compare October 20, 2023 08:00
@findinpath
Copy link
Copy Markdown
Contributor Author

Superseded by #467

@findinpath findinpath closed this Oct 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: FromUnixtimeOperatorTransformer possibly built on false supposisions.

2 participants