Skip to content
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.

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

types: Implement API functions for duration type #135

Merged
merged 9 commits into from
Aug 4, 2024

Conversation

muzarski
Copy link
Collaborator

@muzarski muzarski commented Jul 2, 2024

This PR implements following set of API functions related to duration type:

  • cass_statement_bind_duration_*
  • cass_user_type_set_duration_*
  • cass_tuple_set_duration
  • cass_value_get_duration
  • cass_value_is_duration
  • cass_collection_append_duration

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • PR description sums up the changes and reasons why they should be introduced.
  • [ ] I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
  • [ ] I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

@muzarski
Copy link
Collaborator Author

muzarski commented Jul 2, 2024

@Lorak-mmk @wprzytula Do you know how to enable the tests for the duration type? I see there is a tests/src/integration/values/duration.hpp file but I'm not sure how to enable it in CI.

@muzarski
Copy link
Collaborator Author

muzarski commented Jul 2, 2024

@Lorak-mmk @wprzytula Do you know how to enable the tests for the duration type? I see there is a tests/src/integration/values/duration.hpp file but I'm not sure how to enable it in CI.

I see that the duration tests were run in CI: 12 tests from CassandraTypes/CassandraTypesTests/7, where TypeParam = test::driver::NullableValue<test::driver::values::Duration>. I'm curious why Decimal tests are not being run (decimal is unimplemented type).

@muzarski muzarski self-assigned this Jul 2, 2024
@Lorak-mmk
Copy link
Collaborator

@Lorak-mmk @wprzytula Do you know how to enable the tests for the duration type? I see there is a tests/src/integration/values/duration.hpp file but I'm not sure how to enable it in CI.

To be honest I'm not sure how those tests work. I can see that both duration and decimal have some special handling.
Those tests are defined in tests/src/integration/tests/test_cassandra_types.cpp
I can see that there is some specialization for Duration:

class CassandraTypesDurationTests : public CassandraTypesTests<Duration> {};

And later some registration for types excluding duration:
// Register all parameterized test cases for primitives (excludes duration)
REGISTER_TYPED_TEST_CASE_P(CassandraTypesTests, Integration_Cassandra_Basic,
Integration_Cassandra_ByName, Integration_Cassandra_NamedParameters,
Integration_Cassandra_NullValues, Integration_Cassandra_NullList,
Integration_Cassandra_NullMap, Integration_Cassandra_NullSet,
Integration_Cassandra_List, Integration_Cassandra_Set,
Integration_Cassandra_Map, Integration_Cassandra_Tuple,
Integration_Cassandra_UDT);

There is also some instantiation that includes decimal:
// Instantiate the test case for all the Cassandra data types
typedef testing::Types<Ascii, BigInteger, Blob, Boolean, Date, Decimal, Double, Duration, Float,
Inet, Integer, SmallInteger, Text, Time, Timestamp, TimeUuid, TinyInteger,
Uuid, Varchar, Varint>
CassandraTypes;
INSTANTIATE_TYPED_TEST_CASE_P(CassandraTypes, CassandraTypesTests, CassandraTypes);

@Lorak-mmk
Copy link
Collaborator

@Lorak-mmk @wprzytula Do you know how to enable the tests for the duration type? I see there is a tests/src/integration/values/duration.hpp file but I'm not sure how to enable it in CI.

I see that the duration tests were run in CI: 12 tests from CassandraTypes/CassandraTypesTests/7, where TypeParam = test::driver::NullableValue<test::driver::values::Duration>. I'm curious why Decimal tests are not being run (decimal is unimplemented type).

Where do you see that? I don't see any occurence of decimal or duration in CI logs.

@muzarski
Copy link
Collaborator Author

muzarski commented Jul 2, 2024

@Lorak-mmk @wprzytula Do you know how to enable the tests for the duration type? I see there is a tests/src/integration/values/duration.hpp file but I'm not sure how to enable it in CI.

I see that the duration tests were run in CI: 12 tests from CassandraTypes/CassandraTypesTests/7, where TypeParam = test::driver::NullableValue<test::driver::values::Duration>. I'm curious why Decimal tests are not being run (decimal is unimplemented type).

Where do you see that? I don't see any occurence of decimal or duration in CI logs.

build job, step Run integration tests on Scylla 5.0.0, line 577

@Lorak-mmk
Copy link
Collaborator

I see. Ctrl + f seems to be quite unreliable in those logs...

@muzarski
Copy link
Collaborator Author

v2: rebased on master

@Lorak-mmk Lorak-mmk self-assigned this Jul 30, 2024
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
scylla-rust-wrapper/src/binding.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/query_result.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/query_result.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/value.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/value.rs Outdated Show resolved Hide resolved
@Lorak-mmk Lorak-mmk removed their assignment Jul 31, 2024
@muzarski
Copy link
Collaborator Author

v3:

  • rebased on master
  • addressed minor issues
  • fixed cass_value_is_duration and cass_value_is_collection
  • making use of rust-driver's builtin serialization for simple types (first commit).

scylla-rust-wrapper/src/value.rs Outdated Show resolved Hide resolved
Long ago, duration type was not supported by rust driver. This is not
true anymore.

This commit implements two API functions:
- cass_value_get_duration
- cass_value_is_duration
Implemented binding macros for duration type.

Each duration API function accepts 3 value parameters:
- months: i32
- days: i32
- nanoseconds: i64

They are put into CqlDuration structure which is directly
encapsulated by CqlValue::Duration.
Implemented cass_collection_append_duration.
Implemented binding duration value to statements.
Implemented cass_tuple_set_duration function.
Implemented cass_user_type_set_duration_* functions.
cpp-driver does that check for some reason.
previously, we would not consider null collections
@muzarski
Copy link
Collaborator Author

v3.1: rebased on master

@muzarski muzarski requested a review from wprzytula July 31, 2024 14:10
@Lorak-mmk
Copy link
Collaborator

One more question: before you mentioned some tests that were incorrectly skipped. Is that fixed now?

@muzarski
Copy link
Collaborator Author

muzarski commented Aug 4, 2024

One more question: before you mentioned some tests that were incorrectly skipped. Is that fixed now?

Yes, there is an issue for that: #144. Do you want me to fix it before merging this PR?

@Lorak-mmk
Copy link
Collaborator

It can be a separate PR - but I don't want to postpone it too long.

@Lorak-mmk Lorak-mmk merged commit ee867e3 into scylladb:master Aug 4, 2024
5 checks passed
@muzarski muzarski mentioned this pull request Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants