Renames all hidden built-in functions using unicode non-character #1695
Conversation
partiql-parser/src/main/kotlin/org/partiql/parser/internal/PartiQLParserDefault.kt
Outdated
Show resolved
Hide resolved
| @@ -65,7 +65,7 @@ internal val Fn_DATE_ADD_MONTH__INT_DATE__DATE = Function.static( | |||
| Datum.date(datetimeValue.plusMonths(intervalValue)) | |||
| } | |||
|
|
|||
| internal val Fn_DATE_ADD_MONTH__INT32_TIMESTAMP__TIMESTAMP = Function.static( | |||
| internal val Fn_DATE_ADD_MONTH__INT32_TIMESTAMP__TIMESTAMP = FunctionUtils.hidden( | |||
There was a problem hiding this comment.
See these, I recommend just adding ".hidden" or ".system" to Function rather than FunctionUtils, but I also don't know if you had to deal with visibility modifier issues.
There was a problem hiding this comment.
Yeah, that would just make them public, which I'm hoping to avoid in this PR.
partiql-spi/src/main/kotlin/org/partiql/spi/function/builtins/FnEq.kt
Outdated
Show resolved
Hide resolved
partiql-spi/src/main/kotlin/org/partiql/spi/function/utils/FunctionUtils.kt
Outdated
Show resolved
Hide resolved
partiql-planner/src/main/kotlin/org/partiql/planner/internal/transforms/RexConverter.kt
Outdated
Show resolved
Hide resolved
partiql-spi/src/main/kotlin/org/partiql/spi/function/utils/FunctionUtils.kt
Outdated
Show resolved
Hide resolved
partiql-spi/src/main/kotlin/org/partiql/spi/function/builtins/DiadicOperator.kt
Outdated
Show resolved
Hide resolved
| @@ -1752,9 +1762,10 @@ internal class PartiQLParserDefault : PartiQLParser { | |||
| val rhs = visitExpr(ctx.expr(1)) | |||
| val fieldLit = ctx.dt.text.lowercase() | |||
| // TODO error on invalid datetime fields like TIMEZONE_HOUR and TIMEZONE_MINUTE | |||
| // TODO: This should (likely) be parsed into its own node. The planner should convert this into a call. | |||
There was a problem hiding this comment.
So do we now think that DATE_ADD and DATE_DIFF should be their own nodes in the AST? For some context, we had those special form functions as their own node in v0.14 and prior. Then for the AST migration, we switched to define them as ExprCalls -- #1579 (comment).
With how it's currently modeled, it was somewhat messy to parse and perform other operations like pretty-printing in SqlDialect -- #1638 (comment). And as we can see here, it's messy to port the hidden prefix logic into the parser package.
We could defer this decision to post-v1 since adding a dedicated node for DATE_ADD and DATE_DIFF would be an additive change.
There was a problem hiding this comment.
True. For now, I'm just adding the prefix and leaving the TODO. Post V1, it might be a good idea to reintroduce those nodes or go with the translation that Robert mentioned. I don't have a preference.
partiql-spi/src/main/kotlin/org/partiql/spi/function/builtins/FnExists.kt
Outdated
Show resolved
Hide resolved
| import org.partiql.spi.function.utils.StringUtils.codepointTrimTrailing | ||
| import org.partiql.spi.value.Datum | ||
| import org.partiql.types.PType | ||
|
|
||
| internal val Fn_TRIM_TRAILING__STRING__STRING = Function.static( |
There was a problem hiding this comment.
Seems like some other functions aren't hidden yet. Taking a look at the https://github.com/johnedquinn/partiql-lang-jvm/blob/main-builtins-names/partiql-spi/src/main/kotlin/org/partiql/spi/function/Builtins.kt#L21,
- abs
- bit_length
- cardinality
- char_length
- lower
- (some others)
There was a problem hiding this comment.
These are public in SQL. See Section 20.70 and 6.17.
There was a problem hiding this comment.
I see. Perhaps could be good to add a comment at some place (perhaps in the FunctionUtils.kt) that these SQL99-defined scalar and aggregation function names are purposely not hidden.
| /** | ||
| * Returns an implementation of a [Function] that has the [name] obfuscated via the [SYSTEM_PREFIX_INTERNAL]. | ||
| */ | ||
| fun hidden( |
There was a problem hiding this comment.
Will aggregations also be hidden w/ the special prefix?
There was a problem hiding this comment.
No. See #1695 (comment). Same reasoning.
| internal val Fn_NOT__BOOL__BOOL = object : Function { | ||
|
|
||
| private var name = "not" | ||
| private val name = FunctionUtils.SYSTEM_PREFIX_INTERNAL + "not" |
There was a problem hiding this comment.
There are some functions (like here and IS_MISSING, IS_NULL, etc) that do not directly use the FunctionUtils.hidden method. Is that because they haven't been migrated to use the new interface (from #1646)?
There was a problem hiding this comment.
Not necessarily. They don't have a bunch of overloads, so they don't need to directly implement the new interface.
partiql-spi/src/main/kotlin/org/partiql/spi/function/builtins/FnIsMissing.kt
Outdated
Show resolved
Hide resolved
johnedquinn
force-pushed
the
main-builtins-names
branch
from
5298e40 to
c753887
Compare
|
Overall LGTM and gets that job done, but you may want to consider not having a FunctionUtils and not listing out the names of functions. It would be better to just reference the name on the operator itself. It's not ideal to have the names floating around in multiple places because now the process of adding operators is fragile and requires updating in multiple places which does not scale well. I recommend brainstorming better ways to register and reference builtins that does not require juggling |
Description
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.