Repository navigation
[C++] Preserve TableSchema state during moves - #1032
Open
zhangxinyao88 wants to merge 2 commits into
Open
zhangxinyao88 wants to merge 2 commits into
zhangxinyao88 wants to merge 2 commits into
Conversation
Default the move constructor to retain the column lookup index, virtual-table flag and pending tree depth along with the other members. Cover writer registration and readback after a move, duplicate-column rejection, virtual lookups and tree-level finalization.
Use a complete initializer list for the supported legacy MSVC toolset instead of a defaulted noexcept move constructor. Preserve the index, virtual-table flag and pending tree depth along with the other members. Check measurement ownership in the move regression so that a deep-copy fallback cannot satisfy the column-index assertion.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moving a valid
TableSchemadropscolumn_pos_index_, soTsFileWriter::register_tablerejects it as if it had duplicate columns. The move also resets the virtual-table flag and pending tree depth, changing case-sensitive lookups and losing generated tree-level tag columns.Explicitly move all seven members, including the column index, virtual-table flag and pending tree depth. Keep the constructor explicit for supported MSVC v141 toolchains. Add regressions for registration followed by a write/read round trip, duplicate-column rejection, virtual-table lookup semantics, and tree-level finalization after a move. Four regressions fail on the original develop implementation; the duplicate-name rejection control already passes. The column-index test also checks shared measurement ownership, detecting a deep-copy fallback.
Part of #468, bug20. The other findings in that aggregate report remain separate.
Validation:
./mvnw -B -ntp -P with-cpp -Denable.lzma2=ON verify: 1,168 passed, 3 skipped, 10 disabled, 0 failed; Spotless, Checkstyle and RAT passed.halt_on_error=1. Compiler flags and sanitizer runtime linkage were verified. This separate Debug build disables optional codecs, ANTLR and SIMD; the complete Maven run above enables them, including LZMA2.Local Java, Python and Windows/MSVC tests were not run.
The first defaulted-move implementation failed two tests in actual MSVC 19.16 v141 CI. A small MSVC 19.14 compile reproduces copy-constructor fallback; explicit member initialization emits the intended transfers. Current head
70fcb8b621306724e45b2270b032e4a73b9ab819passes the local checks above. Its upstream MSVC v141 Debug and Release jobs now pass, as do the default-toolset ASan jobs and Python-MSVC jobs. The actual Debug v141 log uses MSVC 19.16.27054 and passes the column-index, virtual-table and pending-tree-level regressions. Overall CI remains non-green: seven Ubuntu setup failures remain (missing clang-format-17 or unavailable Python 3.9), separate from the repaired Windows runtime failures. No compiler upgrade or regression-test suppression is included.