-
Notifications
You must be signed in to change notification settings - Fork 268
fix: Remove fallback for maps containing complex types #2943
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| val partitionSchemaSupported = | ||
| typeChecker.isSchemaSupported(partitionSchema, fallbackReasons) | ||
|
|
||
| def hasUnsupportedType(dataType: DataType): Boolean = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check is no longer needed
| case t: StructType => t.exists(f => hasTemporalType(f.dataType)) | ||
| case t: ArrayType => hasTemporalType(t.elementType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was missing a check for MapType so I added that and moved the method to DataTypeSupport
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2943 +/- ##
============================================
+ Coverage 56.12% 59.63% +3.50%
- Complexity 976 1375 +399
============================================
Files 119 167 +48
Lines 11743 15487 +3744
Branches 2251 2566 +315
============================================
+ Hits 6591 9235 +2644
- Misses 4012 4955 +943
- Partials 1140 1297 +157 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| def hasTemporalType(t: DataType): Boolean = t match { | ||
| case DataTypes.DateType | DataTypes.TimestampType | DataTypes.TimestampNTZType => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if interval can also be considered as temporal? Probably not, it is more like duration rather than representing any point of time
| var filename: String = null | ||
|
|
||
| /** Filename for data file with deeply nested complex types */ | ||
| var complexTypesFilename: String = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be great to add if this filename is input or output or temp location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will improve the comments in my next PR later today
comphead
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @andygrove
|
Thanks for the review @comphead |
Which issue does this PR close?
N/A
Rationale for this change
Ongoing refactoring / fixing technical debt
What changes are included in this PR?
CometFuzzTestBasenow generates an additional Parquet file containing deeply nested typesCometScanRulehandling ofautomodeMapTypeHow are these changes tested?