[ntuple] remove kNTupleUnknownCompression #17389
Conversation
| @@ -166,8 +166,8 @@ ROOT::Experimental::RNTupleInspector::Create(std::string_view ntupleName, std::s | |||
|
|
|||
| std::string ROOT::Experimental::RNTupleInspector::GetCompressionSettingsAsString() const | |||
| { | |||
| int algorithm = fCompressionSettings / 100; | |||
| int level = fCompressionSettings - (algorithm * 100); | |||
| int algorithm = fCompressionSettings.value() / 100; | |||
There was a problem hiding this comment.
Shouldn't this properly handle the case where compression is unknown, rather than throwing?
| @@ -162,7 +162,7 @@ RNTupleExporter::RPagesResult RNTupleExporter::ExportPages(RPageSource &source, | |||
| const std::uint64_t pageBufSize = pageInfo.fLocator.GetNBytesOnStorage() + maybeChecksumSize; | |||
| std::ostringstream ss{options.fOutputPath, std::ios_base::ate}; | |||
| ss << "/cluster_" << clusterDesc.GetId() << "_" << colInfo.fQualifiedName << "_page_" << pageIdx | |||
| << "_elems_" << pageInfo.fNElements << "_comp_" << colRange.fCompressionSettings << ".page"; | |||
| << "_elems_" << pageInfo.fNElements << "_comp_" << colRange.fCompressionSettings.value() << ".page"; | |||
There was a problem hiding this comment.
Shouldn't this properly handle the case where compression is unknown, rather than throwing?
| @@ -212,7 +213,7 @@ public: | |||
| /// | |||
| /// \note Here, we assume that the compression settings are consistent across all clusters and columns. If this is | |||
| /// not the case, an exception will be thrown when RNTupleInspector::Create is called. | |||
| int GetCompressionSettings() const { return fCompressionSettings; } | |||
| int GetCompressionSettings() const { return fCompressionSettings.value(); } | |||
There was a problem hiding this comment.
Is there a reason why this shouldn't return an optional?
The comment states that "we assume that the compression settings are consistent across all clusters and columns" - do we also assume the compression settings are not unknown? Perhaps we should add a brief explanation on why fCompressionSettings is an optional, it might not be obvious to a future reader.
jblomer
force-pushed
the
ntuple-cleanup-utils-v2
branch
from
b275935 to
9f88206
Compare
Test Results 16 files 16 suites 3d 17h 50m 20s ⏱️ For more details on these failures, see this check. Results for commit 9f88206. |
Instead represent undefined/unknown compression settings by std::optional<>. Also allows for representing compression settings as unsigned integer.