fix: classify unsupported format patterns as Unsupported in CometFromUnixTime#4660
fix: classify unsupported format patterns as Unsupported in CometFromUnixTime#4660marvelshan wants to merge 1 commit into
Conversation
0aa73b7 to
32f11be
Compare
| val timeZone = exprToProtoInternal(Literal(expr.timeZoneId.orNull), inputs, binding) | ||
|
|
||
| if (expr.format != Literal(TimestampFormatter.defaultPattern)) { | ||
| if (expr.format != Literal(TimestampFormatter.defaultPattern())) { |
There was a problem hiding this comment.
nit : might be an unwanted change ?
| if (expr.format != Literal(TimestampFormatter.defaultPattern())) { | ||
| Unsupported(Some("Only the default datetime pattern `yyyy-MM-dd HH:mm:ss` is supported")) | ||
| } else { | ||
| Incompatible(None) |
There was a problem hiding this comment.
We might wnat to add some SQL tests to verify the fallback and make sure the plan is annotated
beeda26 to
1a8dea4
Compare
|
|
||
| override def getSupportLevel(expr: FromUnixTime): SupportLevel = { | ||
| if (expr.format != Literal(TimestampFormatter.defaultPattern())) { | ||
| Unsupported(Some("Only the default datetime pattern `yyyy-MM-dd HH:mm:ss` is supported")) |
There was a problem hiding this comment.
I think this change may prevent the codegen dispatch path from working.
| SELECT from_unixtime(t) FROM test_from_unix_time | ||
|
|
||
| query | ||
| query expect_fallback(Only the default datetime pattern `yyyy-MM-dd HH:mm:ss` is supported) |
There was a problem hiding this comment.
This query was previously working (presumably due to codegen dispatch). This seems like a regression.
1a8dea4 to
35ebc98
Compare
|
Thanks @marvelshan. The instinct here is good. Separating the genuine timestamp-range incompatibility from the format-pattern caveat is the right idea, and pulling the format text out of A few things I would like to work through before this lands. 1. The code keeps this override def getSupportLevel(expr: FromUnixTime): SupportLevel = {
if (expr.format != Literal(TimestampFormatter.defaultPattern())) {
Incompatible(Some("Only the default datetime pattern ... is supported natively"))
} else {
Incompatible(None)
}
}I actually think keeping it 2. 3. The non-default test downgrade looks inconsistent. In 4. Minor. The Disclosure: this review was assisted by an AI tool (Anthropic Claude via Claude Code). I read the diff, the linked issues, and the related serde code before forming these comments, and I take responsibility for them. |
2ac73c8 to
cba01ac
Compare
…ormats through codegen dispatcher - Move non-default format pattern limitation from getIncompatibleReasons to getCompatibleNotes to avoid misleading 'Unsupported' heading in generated docs, following the date_format pattern - Keep getSupportLevel as Incompatible for non-default formats to preserve the codegen dispatch path - Route non-default format patterns through CometScalaUDF.emitJvmCodegenDispatch instead of returning None, matching how DateFormatClass handles non-native paths - Change from_unix_time.sql and from_unix_time_enabled.sql non-default format tests from spark_answer_only to query since codegen dispatcher handles them natively - Make defaultPattern() call consistent between getSupportLevel and convert
cba01ac to
a772485
Compare
Which issue does this PR close?
Closes #4575
Rationale for this change
CometFromUnixTimeincorrectly reports non-default datetime format patterns asIncompatibleinstead ofUnsupported. This is misleading because:Incompatibleimplies the expression can run natively if the user opts in viaspark.comet.expr.allowIncompatible=true, but non-default formats cannot be executed natively at all — theconvert()method returnsNonefor them regardless of configuration.GenerateDocstool usesgetIncompatibleReasons()andgetUnsupportedReasons()to produce the public Compatibility Guide. Classifying format limitations as "incompatible" misleads users into thinking they can enable support by settingallowIncompatible=true.What changes are included in this PR?
getIncompatibleReasons()to only contain the timestamp range difference (which is a true incompatibility — native can execute but results may differ at boundaries)getUnsupportedReasons()to document format pattern limitations (native cannot execute these at all)getSupportLevel(expr)to dynamically returnUnsupportedfor non-default format patterns andIncompatiblefor the default pattern, matching the actual behavior inconvert()How are these changes tested?
Existing tests cover the fallback behavior. The
convert()method already returnsNonefor non-default formats, andgetSupportLevel()now aligns with that behavior. TheGenerateDocsoutput will correctly classify format limitations under "Unsupported" rather than "Incompatible".