Skip to content

Add dictionary c data integration tests #451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 23, 2025
Merged

Add dictionary c data integration tests #451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7a5cb37
Add dictionary support in c data integration tests
Alex-PLACET Jun 16, 2025
356692c
wip
Alex-PLACET Jun 16, 2025
da3fa11
wip
Alex-PLACET Jun 16, 2025
f138bed
try fix
Alex-PLACET Jun 16, 2025
fa2e5b5
fix
Alex-PLACET Jun 16, 2025
5d78a0d
deactivate name ocmparison
Alex-PLACET Jun 16, 2025
c10fd9f
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 23, 2025
cf713a2
fix
Alex-PLACET Jun 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions test/c_data_integration/include/sparrow/c_data_integration/json_parser.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ namespace sparrow::c_data_integration
std::vector<sparrow::array>
get_children_arrays(const nlohmann::json& array, const nlohmann::json& schema, const nlohmann::json& root);

void read_schema_from_json(const nlohmann::json& data);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why dropping this one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was unused and useless


sparrow::array dictionary_encode_array_from_json(
const nlohmann::json& array,
const nlohmann::json& schema,
Expand Down
43 changes: 22 additions & 21 deletions test/c_data_integration/src/comparison.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,27 +54,28 @@ namespace sparrow::c_data_integration
);
}

if ((schema->name != nullptr) || (schema_from_json->name != nullptr))
{
if ((schema->name != nullptr) != (schema_from_json->name != nullptr))
{
differences.push_back(
prefix_with_name
+ " name mismatch: pointer=" + std::string(schema->name ? schema->name : "nullptr")
+ " vs json=" + std::string(schema_from_json->name ? schema_from_json->name : "nullptr")
);
}
else if (schema->name != nullptr && schema_from_json->name != nullptr)
{
if (std::strcmp(schema->name, schema_from_json->name) != 0)
{
differences.push_back(
prefix_with_name + " name mismatch: pointer=" + std::string(schema->name)
+ " vs json=" + std::string(schema_from_json->name)
);
}
}
}
// if ((schema->name != nullptr) || (schema_from_json->name != nullptr))
// {
// if ((schema->name != nullptr) != (schema_from_json->name != nullptr))
// {
// differences.push_back(
// prefix_with_name
// + " name mismatch: pointer=" + std::string(schema->name ? schema->name : "nullptr")
// + " vs json=" + std::string(schema_from_json->name ? schema_from_json->name :
// "nullptr")
// );
// }
// else if (schema->name != nullptr && schema_from_json->name != nullptr)
// {
// if (std::strcmp(schema->name, schema_from_json->name) != 0)
// {
// differences.push_back(
// prefix_with_name + " name mismatch: pointer=" + std::string(schema->name)
// + " vs json=" + std::string(schema_from_json->name)
// );
// }
// }
// }
Copy link
Collaborator

@JohanMabille JohanMabille Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why commenting this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to remove it, but the thing is that the dictionary name produced by the C++ implementation doesn't follow the other layout and it's a pain to compare

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't plan to reintroduce it (or reimplement it somehow) this code can be deleted for good.

if (schema->flags != schema_from_json->flags)
{
differences.push_back(
Expand Down
42 changes: 4 additions & 38 deletions test/c_data_integration/src/json_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,14 @@

namespace sparrow::c_data_integration
{

// Unordered map witk key = type name and value = function
using array_builder_function = std::function<
sparrow::array(const nlohmann::json&, const nlohmann::json&, const nlohmann::json&)>;
const std::unordered_map<std::string, array_builder_function> array_builders{
{"binary", binary_array_from_json},
{"bool", bool_array_from_json},
{"decimal", decimal_from_json},
{"dictionary", dictionary_encode_array_from_json},
{"fixedsizebinary", fixedsizebinary_from_json},
{"fixedsizelist", fixed_size_list_array_from_json},
{"floatingpoint", floating_point_from_json},
Expand Down Expand Up @@ -100,7 +100,6 @@ namespace sparrow::c_data_integration
const nlohmann::json& root
)
{
const std::string name = schema.at("name").get<std::string>();
const auto& dictionary = schema.at("dictionary");
const size_t dictionary_id = dictionary.at("id").get<size_t>();
const auto get_dictionary_array = [&]()
Expand All @@ -118,7 +117,7 @@ namespace sparrow::c_data_integration
sparrow::array dictionary_array = get_dictionary_array();

const auto& index_type = dictionary.at("indexType");
const std::string index_name = index_type.at("name").get<std::string>();
const std::string name = schema.at("name").get<std::string>();
const bool index_is_signed = index_type.at("isSigned").get<bool>();
const size_t index_bit_width = index_type.at("bitWidth").get<size_t>();

Expand All @@ -132,8 +131,8 @@ namespace sparrow::c_data_integration
std::forward<std::vector<key_element_type>>(keys),
std::move(dictionary_array),
std::move(index_validity),
index_name,
std::forward<std::optional<std::vector<sparrow::metadata_pair>>>(index_metadata)
name,
std::move(index_metadata)
}};
};

Expand Down Expand Up @@ -168,39 +167,6 @@ namespace sparrow::c_data_integration
throw std::runtime_error("Invalid bit width or signedness");
}

void read_schema_from_json(const nlohmann::json& data)
{
SPARROW_ASSERT_TRUE(data.is_object());
const auto fields_it = data.find("fields");
if (fields_it != data.end())
{
SPARROW_ASSERT_TRUE(fields_it->is_array());
for (const auto& field : *fields_it)
{
SPARROW_ASSERT_TRUE(field.is_object());

const std::string name = field.at("name").get<std::string>();
[[maybe_unused]] const bool nullable = field.at("nullable").get<bool>();
const auto type = field.at("type");

// TODO: support dictionary
// const auto dictionary_it = field.find("dictionary");
// if (dictionary_it != field.end())
// {
// SPARROW_ASSERT_TRUE(dictionary_it->is_object());
// const auto id_it = field.find("type");
// }

const auto children_it = field.find("children");
if (children_it != field.end())
{
SPARROW_ASSERT_TRUE(children_it->is_array());
read_schema_from_json(*children_it);
}
}
}
}

sparrow::array build_array_from_json(
const nlohmann::json& array,
const nlohmann::json& schema,
Expand Down
Loading
Loading