[KERNEL] Kernel fix clustering and add tests#6749
Conversation
| val before = table.getLatestSnapshot(engine).asInstanceOf[SnapshotImpl] | ||
| val featuresBefore = before.getProtocol.getImplicitlyAndExplicitlySupportedFeatures | ||
| assert( | ||
| !featuresBefore.contains(io.delta.kernel.internal.tablefeatures.TableFeatures |
There was a problem hiding this comment.
There are several fully-qualified references in tests (e.g. io.delta.kernel.internal.tablefeatures.TableFeatures.GEOSPATIAL_RW_FEATURE and io.delta.kernel.internal.TableConfig.CHECKPOINT_POLICY). To avoid repeating inline several times, could we add the corresponding imports and tighten readability a bit?
uros-db
left a comment
uros-db
left a comment
There was a problem hiding this comment.
The new clustering check inspects top-level columns, but how about clustering on a nested geo field? Let's confirm that and perhaps consider adding appropriate tests
uros-db
left a comment
uros-db
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments! @scottsand-db please review
| assert(after.getProtocol.getMinReaderVersion >= 3) | ||
| assert(after.getProtocol.getMinWriterVersion >= 7) |
There was a problem hiding this comment.
| assert(after.getProtocol.getMinReaderVersion >= 3) | |
| assert(after.getProtocol.getMinWriterVersion >= 7) | |
| assert(after.getProtocol.getMinReaderVersion == 3) | |
| assert(after.getProtocol.getMinWriterVersion == 7) |
| .fromMetadata(getMetadata(engine, tablePath)) | ||
| } | ||
|
|
||
| // Adding a geo column auto-enables GEOSPATIAL_RW_FEATURE and bumps protocol to (3, 7). |
There was a problem hiding this comment.
should we add test cover the case where schema evolution reject the GeometryType due to malformed GeographyType, e.g GeographyType("XyZ", "vincenty") ?
3 participants
Which Delta project/connector is this regarding?
Description
In this PR we are adding additional testing for geospatial types for kernel:
We've also fixed issue where clustering was accidentaly passing.
How was this patch tested?
New tests
Does this PR introduce any user-facing changes?
Forbids clustering on geo columns