wangyong9999 commented on code in PR #245:
URL: https://github.com/apache/paimon-cpp/pull/245#discussion_r3851792863
##########
src/paimon/common/utils/fields_comparator.cpp:
##########
@@ -128,21 +144,24 @@ Result<FieldsComparator::FieldComparatorFunc>
FieldsComparator::CompareField(
return lvalue == rvalue ? 0 : (lvalue < rvalue ? -1 : 1);
});
case arrow::Type::type::FLOAT:
- // TODO(xinyu.lxy):
- // currently in java KeyComparatorSupplier: -inf < -0.0 == +0.0 <
+inf = nan
- // paimon-cpp: -inf < -0.0 == +0.0 < +inf and nan cannot be
compared
return FieldsComparator::FieldComparatorFunc(
- [field_idx](const InternalRow& lhs, const InternalRow& rhs) ->
int32_t {
+ [field_idx, use_java_floating_point_order](const InternalRow&
lhs,
+ const InternalRow&
rhs) -> int32_t {
float lvalue = lhs.GetFloat(field_idx);
float rvalue = rhs.GetFloat(field_idx);
- return lvalue == rvalue ? 0 : (lvalue < rvalue ? -1 : 1);
+ return use_java_floating_point_order
+ ? CompareFloatingPoint(lvalue, rvalue)
+ : (lvalue == rvalue ? 0 : (lvalue < rvalue ? -1
: 1));
Review Comment:
Scoping the Java order to the index build is the right call, but note what
the `false` branch is: `compare(NaN, x)` and `compare(x, NaN)` both return 1,
so it is not a strict weak ordering, and every `std::sort` / `std::stable_sort`
/ heap comparator that consumes a default `FieldsComparator` is UB the moment a
NaN shows up.
Leaving that behavior alone in this PR is fine, but the TODO that documented
it ("nan cannot be compared") was deleted in the previous revision, so the
branch now reads as deliberate and correct. Worth restoring a note here.
##########
src/paimon/core/operation/expire_snapshots.cpp:
##########
@@ -144,7 +156,33 @@ Result<int32_t> ExpireSnapshots::ExpireUntil(int64_t
earliest_snapshot_id,
continue;
}
PAIMON_ASSIGN_OR_RAISE(Snapshot snapshot,
snapshot_manager_->LoadSnapshot(id));
-
PAIMON_RETURN_NOT_OK(CleanUnusedDataFiles(snapshot.DeltaManifestList()));
+ bool tag_changed = false;
+ while (next_tag != tagged_snapshots.end() && next_tag->Id() < id) {
+ previous_tag = &*next_tag;
+ ++next_tag;
+ tag_changed = true;
+ }
+ if (tag_changed) {
+ Result<std::set<std::string>> tagged_data_files_result =
+ GetTaggedDataFiles(*previous_tag);
+ if (!tagged_data_files_result.ok()) {
+ PAIMON_LOG_WARN(logger_,
+ "Skip cleaning data files of snapshot #%ld
because the data files "
+ "referenced by tag snapshot #%ld could not be
loaded. %s",
+ id, previous_tag->Id(),
+
tagged_data_files_result.status().ToString().c_str());
+ tagged_data_files.reset();
+ } else {
+ tagged_data_files =
std::move(tagged_data_files_result).value();
+ }
+ }
+ if (previous_tag != nullptr && !tagged_data_files) {
Review Comment:
Two notes on this skip:
- `tagged_data_files` is only recomputed when the tag changes, so one failed
read is sticky for every remaining snapshot under the same tag, not just this
one. Java retries `tryReadDataFiles` per expiring snapshot and only caches
successes.
- The skip covers data-file cleanup only. The second loop still deletes this
snapshot's manifests and the snapshot file itself, so the data files that were
not cleaned end up unreachable — no snapshot references them and no delta
manifest lists them any more. They are recoverable only through orphan cleanup.
The conservative direction is right (better an orphan than a deleted tagged
file); worth saying the second part in the warning so the leak is diagnosable.
##########
src/paimon/core/schema/schema_validation.cpp:
##########
@@ -341,6 +402,64 @@ 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(),
Options::PK_CLUSTERING_OVERRIDE, false));
+ if (pk_clustering_override) {
Review Comment:
`FileStoreCommitImpl::ValidateCommitOptions` already rejects
`pk-clustering-override` for every table (`file_store_commit.cpp:129` runs it
on every commit path), so this branch only fires for a schema whose first
commit would fail anyway, and the message reads as if the option works when
BTree indexes are off. Either drop it or point it at the global restriction.
--
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]
