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

C++ driver UX improvements #541

Merged

Conversation

krishnangovindraj
Copy link
Member

@krishnangovindraj krishnangovindraj commented Dec 14, 2023

Usage and product changes

Add a few missing methods, and function variants for UX.

Implementation

  • Fixes the (DY)LD_LIBRARY_PATH in the Mac circle assembly tests
  • Avoids using parentheses around commands in circleci config
  • UX changes:
    • Renames drop() methods to deleteX
    • Default arguments for QueryManager,
    • initializer_list<Annotation> variants for ConceptAPI
  • iterator traits, which don't do much but encounter more specific errors on unsupported usage of the iterator.
  • Hides away some static methods as regular methods in the CPP file
  • Updates typedb_driver.i to free the object returned by concept_map_group_to_string and value_map_group_to_string

@typedb-bot
Copy link
Member

typedb-bot commented Dec 14, 2023

PR Review Checklist

Do not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed.


Trivial Change

  • This change is trivial and does not require a code or architecture review.

Code

  • Packages, classes, and methods have a single domain of responsibility.
  • Packages, classes, and methods are grouped into cohesive and consistent domain model.
  • The code is canonical and the minimum required to achieve the goal.
  • Modules, libraries, and APIs are easy to use, robust (foolproof and not errorprone), and tested.
  • Logic and naming has clear narrative that communicates the accurate intent and responsibility of each module (e.g. method, class, etc.).
  • The code is algorithmically efficient and scalable for the whole application.

Architecture

  • Any required refactoring is completed, and the architecture does not introduce technical debt incidentally.
  • Any required build and release automations are updated and/or implemented.
  • Any new components follows a consistent style with respect to the pre-existing codebase.
  • The architecture intuitively reflects the application domain, and is easy to understand.
  • The architecture has a well-defined hierarchy of encapsulated components.
  • The architecture is extensible and scalable.

Sorry, something went wrong.

Copy link
Member Author

@krishnangovindraj krishnangovindraj left a comment

Choose a reason for hiding this comment

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

self review

@@ -460,7 +460,7 @@ commands:
)
tool/test/start-core-server.sh
sleep 3
(cd test_assembly_cpp && LD_LIBRARY_PATH=$ASSEMBLY/lib ./test_assembly && export TEST_SUCCESS=0 || export TEST_SUCCESS=1)
(cd test_assembly_cpp && DYLD_LIBRARY_PATH=$ASSEMBLY/lib ./test_assembly && export TEST_SUCCESS=0 || export TEST_SUCCESS=1)
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand how the test CI assembly test passed. Would it not have reached the branch of the ||?
https://app.circleci.com/pipelines/github/vaticle/typedb-driver/444/workflows/16c41bb4-012f-483b-8e37-c26d2ec3b787/jobs/4048

Copy link
Member

Choose a reason for hiding this comment

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

The parentheses make the command run in a subprocess, I don't believe TEST_SUCCESS is visible outside it. Remove the parens.

@@ -220,7 +220,7 @@ pub extern "C" fn value_group_drop(value_group: *mut ValueGroup) {
}

#[no_mangle]
pub extern "C" fn value_group_to_string(value_group: *const ValueGroup) -> *const c_char {
pub extern "C" fn value_group_to_string(value_group: *const ValueGroup) -> *mut c_char {
Copy link
Member Author

Choose a reason for hiding this comment

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

@dmitrii-ubskii , Was there a reason these were const char* ? It stops me from calling string_free, but I do not how the other languages would have managed to free it.

Copy link
Member

Choose a reason for hiding this comment

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

No, these should be *mut c_char

Copy link
Member

Choose a reason for hiding this comment

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

These now also need to be marked as %newobject in .i so that the strings are GC'd correctly.

using pointer = T*;
using reference = T&;
using iterator_category = std::input_iterator_tag;

Copy link
Member Author

@krishnangovindraj krishnangovindraj Dec 14, 2023

Choose a reason for hiding this comment

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

I'm a little unsure of the difference_type, but found this on stack overflow

I want to introduce this, because when we try:

TypeDBIterable<...> i = ...;
std::vector v<SomeType>(i.begin(), i.end());

Would fail with

error: no matching constructor for initialization of 'std::vector<TypeDB::Database>'
note: candidate template ignored: substitution failure [with _InputIterator = TypeDB::Iterator<TypeDB::_native::DatabaseIterator, TypeDB::_native::Database, TypeDB::Database>]: no type named 'reference' in 'std::iterator_traits<TypeDB::Iterator<TypeDB::_native::DatabaseIterator, TypeDB::_native::Database, TypeDB::Database>>'
  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI vector(_InputIterator __first, _InputIterator __last);

Now it fails with:

error: no matching constructor for initialization of 'std::vector<TypeDB::Database>'
note: candidate template ignored: requirement 'is_constructible<TypeDB::Database, TypeDB::Database &>::value' was not satisfied [with _InputIterator = TypeDB::Iterator<TypeDB::_native::DatabaseIterator, TypeDB::_native::Database, TypeDB::Database>]
  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI vector(_InputIterator __first, _InputIterator __last);

Which makes sense if this methods needs a copy-constructor and we don't allow copying.

std::vector<Annotation> annotations;
annotations.push_back(Annotation::key());
auto keys = collect(context.vars[matches[2].str()]->asThing()->getHas(context.transaction(), annotations));
auto keys = collect(context.vars[matches[2].str()]->asThing()->getHas(context.transaction(), {Annotation::key()}));
Copy link
Member Author

Choose a reason for hiding this comment

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

Use of Initializer_list variant

@@ -55,6 +55,7 @@ class Driver {
DatabaseManager databases;
UserManager users;

static void initLogging();
Copy link
Member Author

Choose a reason for hiding this comment

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

Added initLogging to driver

private:
VoidFuture setOwnsNative(Transaction& transaction, AttributeType* attributeType, AttributeType* overriddenTypeNative, const std::vector<Annotation>& annotations);
ConceptIterable<AttributeType> getOwnsNative(Transaction& transaction, ValueType* valueType, const std::vector<Annotation>& annotations, Transitivity transitivity);

Copy link
Member Author

Choose a reason for hiding this comment

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

Realised I can hide these away altogether.

Copy link
Member

@dmitrii-ubskii dmitrii-ubskii left a comment

Choose a reason for hiding this comment

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

Approved pending a couple comments.

@@ -220,7 +220,7 @@ pub extern "C" fn value_group_drop(value_group: *mut ValueGroup) {
}

#[no_mangle]
pub extern "C" fn value_group_to_string(value_group: *const ValueGroup) -> *const c_char {
pub extern "C" fn value_group_to_string(value_group: *const ValueGroup) -> *mut c_char {
Copy link
Member

Choose a reason for hiding this comment

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

No, these should be *mut c_char

@@ -220,7 +220,7 @@ pub extern "C" fn value_group_drop(value_group: *mut ValueGroup) {
}

#[no_mangle]
pub extern "C" fn value_group_to_string(value_group: *const ValueGroup) -> *const c_char {
pub extern "C" fn value_group_to_string(value_group: *const ValueGroup) -> *mut c_char {
Copy link
Member

Choose a reason for hiding this comment

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

These now also need to be marked as %newobject in .i so that the strings are GC'd correctly.

@@ -460,7 +460,7 @@ commands:
)
tool/test/start-core-server.sh
sleep 3
(cd test_assembly_cpp && LD_LIBRARY_PATH=$ASSEMBLY/lib ./test_assembly && export TEST_SUCCESS=0 || export TEST_SUCCESS=1)
(cd test_assembly_cpp && DYLD_LIBRARY_PATH=$ASSEMBLY/lib ./test_assembly && export TEST_SUCCESS=0 || export TEST_SUCCESS=1)
Copy link
Member

Choose a reason for hiding this comment

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

The parentheses make the command run in a subprocess, I don't believe TEST_SUCCESS is visible outside it. Remove the parens.

@krishnangovindraj krishnangovindraj merged commit 5afefd3 into typedb:development Dec 14, 2023
dmitrii-ubskii pushed a commit to dmitrii-ubskii/typedb-driver that referenced this pull request Jan 8, 2024
## Usage and product changes
Add a few missing methods, and function variants  for UX.

## Implementation
* **Fixes** the `(DY)LD_LIBRARY_PATH` in the Mac circle assembly tests 
* Avoids using parentheses around commands in circleci config
* UX changes:
  - Renames `drop()` methods to `deleteX`
  - Default arguments for QueryManager, 
  - `initializer_list<Annotation>` variants for ConceptAPI
* iterator traits, which don't do much but encounter more specific errors on unsupported usage of the iterator.
* Hides away some static methods as regular methods in the CPP file 
* Updates `typedb_driver.i` to free the object returned by `concept_map_group_to_string` and `value_map_group_to_string`
@krishnangovindraj krishnangovindraj deleted the cpp-driver-ux-changes branch February 16, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants