malinjawi commented on PR #12388: URL: https://github.com/apache/gluten/pull/12388#issuecomment-5015174080
> > It seems that, rather than fixing TIMESTAMP_NTZ itself, we may need to make Gluten Delta handle fallbacks correctly. What do you think? > > Yes, unsupported operations should also induce fallback for Delta plans. @felipepessoto would you file an issue accordingly? > > By the way, will the new tests introduced in this PR overlap the existing ones in Gluten's test codebase? Can we remove the overlapped ones? > > cc @malinjawi @zhztheplayer @felipepessoto Sorry for the delay. I went through our test tree to give you a concrete answer. Yes there's overlap, but it's smaller than it looks. The overlapped ones are the 8 suite files we imported from delta-io/delta over time (`DeltaSuite`, `DeleteSQLSuite` and its DV variants, `UpdateSQLSuite`, `DeltaDDLSuite`, `DeltaInsertIntoTableSuite`,`ClusteredTableClusteringSuite`, `OptimizeMetadataOnlyDeltaQuerySuite`, `OptimizedWritesSuite`). And even those are only redundant on the Spark 4.1 leg, because this pipeline runs Delta 4.2 + Spark 4.1 only. Today those copies are the only Delta tests we have on Spark 3.5 and 4.0. If we delete them now, the older Spark lines lose Delta coverage completely. Everything else is not overlap. `DeltaNativeWriteSuite`, `DeltaDeletionVectorHandoffSuite`, `DeltaDeletionVectorScanInfoSuite`, `DeltaVariantWriteSuite`, the plan-assertion tests in gluten-delta and `VeloxTPCHDeltaSuite` all check Gluten behavior (offload, fallback, DV handoff) that upstream's suites simply don't test. Those stay no matter what. One caveat before we ever delete the copies: they aren't verbatim imports. They carry a lot of triage knowledge, like the `DeltaExcludedTestMixin` exclusion lists with per-test reasons, and assertions changed to `intercept[Exception with SparkThrowable]` because exceptions that cross the JNI boundary come back as different types. If we drop the files without porting those notes into known-failures.txt comments, we lose the "why" behind each divergence. So I'm with @felipepessoto : keep everything until this has baked, then remove in stages. The Spark 4.1 exposure of the src-delta40 copies is the natural first candidate, and we can go further if the pipeline ever grows a 3.5/4.0 leg (though his shard-cost point is fair). I'm happy to do the removal pass when the time comes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
