wangyong9999 commented on code in PR #245:
URL: https://github.com/apache/paimon-cpp/pull/245#discussion_r3850615971
##########
src/paimon/core/schema/schema_validation.cpp:
##########
@@ -341,6 +403,71 @@ Status SchemaValidation::ValidateForDeletionVectors(const
CoreOptions& options)
"no deletion of old data in this merge engine.");
}
+Status SchemaValidation::ValidatePrimaryKeyBTreeIndexes(const TableSchema&
schema,
+ const CoreOptions&
options) {
+ std::vector<std::string> index_columns =
PrimaryKeyBTreeIndexColumns(schema.Options());
+ if (index_columns.empty()) {
+ return Status::OK();
+ }
+
+ PAIMON_ASSIGN_OR_RAISE(PrimaryKeyIndexDefinitions definitions,
+ PrimaryKeyIndexDefinitions::Create(schema));
+ if (!options.DeletionVectorsEnabled()) {
+ return Status::Invalid(
+ "Primary-key BTree indexes require deletion-vectors.enabled =
true.");
+ }
+ if (schema.PrimaryKeys().empty()) {
+ return Status::Invalid("Primary-key BTree indexes require a
primary-key table.");
+ }
+ if (options.GetBucket() <= 0 && !IsPostponeBucketTable(schema,
options.GetBucket())) {
+ return Status::Invalid(
+ fmt::format("Primary-key BTree indexes require fixed or postpone
bucket mode "
+ "(bucket > 0 or bucket = -2), but bucket is {}.",
+ options.GetBucket()));
+ }
+ PAIMON_ASSIGN_OR_RAISE(
+ bool deletion_vectors_merge_on_read,
+ OptionsUtils::GetValueFromMap<bool>(schema.Options(),
kDeletionVectorsMergeOnRead, false));
+ if (deletion_vectors_merge_on_read) {
+ return Status::Invalid(
+ "Primary-key BTree indexes require deletion-vectors.merge-on-read
= false.");
+ }
+ PAIMON_ASSIGN_OR_RAISE(
+ bool pk_clustering_override,
+ OptionsUtils::GetValueFromMap<bool>(schema.Options(),
kPkClusteringOverride, false));
+ if (pk_clustering_override) {
+ return Status::Invalid("Primary-key BTree indexes do not support
pk-clustering-override.");
+ }
+
+ for (const std::string& column : index_columns) {
+ auto field_iter =
+ std::find_if(schema.Fields().begin(), schema.Fields().end(),
+ [&column](const DataField& field) { return
field.Name() == column; });
+ if (field_iter == schema.Fields().end()) {
+ return Status::Invalid(fmt::format("{} entry '{}' must reference
an existing column.",
+
Options::PK_BTREE_INDEX_COLUMNS, column));
+ }
+ if (!IsSupportedBTreeIndexType(field_iter->Type())) {
+ return Status::Invalid(fmt::format("{} entry '{}' has unsupported
type {}.",
+
Options::PK_BTREE_INDEX_COLUMNS, column,
+
field_iter->Type()->ToString()));
+ }
+
+ auto definition_iter = std::find_if(
+ definitions.Definitions().begin(), definitions.Definitions().end(),
+ [&column](const PrimaryKeyIndexDefinition& definition) {
+ return definition.GetFamily() ==
PrimaryKeyIndexDefinition::Family::BTREE &&
+ definition.Column() == column;
+ });
+ if (definition_iter == definitions.Definitions().end()) {
+ return Status::Invalid(
+ fmt::format("Failed to resolve primary-key BTree index column
'{}'.", column));
Review Comment:
Unreachable: `PrimaryKeyIndexDefinitions::Create` builds BTREE definitions
by iterating `schema.Fields()` and testing membership in the btree column list,
so any column that passed the "must reference an existing column" check a few
lines above necessarily has a definition here.
Also, `kPkClusteringOverride` duplicates the same literal already defined in
`file_store_commit_impl.cpp:98`.
##########
src/paimon/core/mergetree/in_memory_sort_buffer.h:
##########
@@ -57,7 +57,8 @@ class InMemorySortBuffer : public SortBuffer {
const std::vector<std::string>&
user_defined_sequence_fields,
bool sequence_fields_ascending,
const std::shared_ptr<FieldsComparator>& key_comparator,
- uint64_t write_buffer_size, const
std::shared_ptr<MemoryPool>& pool);
+ uint64_t write_buffer_size, const
std::shared_ptr<MemoryPool>& pool,
+ const std::shared_ptr<FieldsComparator>&
sort_comparator = nullptr);
Review Comment:
Defaulting to nullptr leaves the main write path on the divergence this
parameter exists to fix: `WriteBuffer` sorts its in-memory runs with Arrow
`SortIndices`, while `ExternalSortBuffer` merges the spilled runs with
`FieldsComparator`. The two orderings must agree or a spilled flush produces a
run that is not globally sorted.
The `CompareFloatingPoint` change in this PR makes them definitively
opposite for NaN — Arrow places NaN with nulls under `NullPlacement::AtStart`,
`FieldsComparator` now places NaN last — so a float/double sort field
containing NaN merges incorrectly once spilling kicks in.
Separately, that change also makes `-0.0 < +0.0` where the old comparator
returned 0, which changes key equality for every existing table with a
float/double key or sequence field. Worth splitting out, or at least calling
out in the description.
##########
src/paimon/core/operation/file_system_write_restore.h:
##########
@@ -84,9 +84,19 @@ class FileSystemWriteRestore : public WriteRestore {
partition, bucket));
}
+ std::vector<std::shared_ptr<IndexFileMeta>> primary_key_index_payloads;
+ if (scan_primary_key_indexes) {
+ if (index_file_handler_ == nullptr) {
+ return Status::Invalid("Primary-key index restore requires an
index file handler.");
+ }
+ PAIMON_ASSIGN_OR_RAISE(
+ primary_key_index_payloads,
+ index_file_handler_->Scan(snapshot.value(), "btree",
partition, bucket));
Review Comment:
Third copy of this literal — `BtreeDefs::kIdentifier` and `kBTreeIndexType`
(primary_key_index_definitions.cpp:38) already exist, and the value has to stay
in sync with `PrimaryKeyIndexDefinition::IndexType()` or the restore scan
silently returns nothing and every level gets rebuilt on each commit. A generic
`WriteRestore` hard-coding one index family is also the wrong layer; the index
type could come from the maintainer factory, which already supplies the
`IndexFileHandler`.
While here: `scan_primary_key_indexes = false` is a default argument on a
virtual function (declared on both the pure virtual in `write_restore.h` and
this override). Defaults bind statically, so a future override with a different
default silently changes behavior depending on the static type. Better to make
it required and update the two call sites.
--
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]
