Skip to content

RDBC-670 Streaming #238

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

Open
wants to merge 3 commits into
base: v7.0
Choose a base branch
from
Open

RDBC-670 Streaming #238

wants to merge 3 commits into from

Conversation

yh-0
Copy link

@yh-0 yh-0 commented May 15, 2025

Rebased changes from #168
Fixed issue with duplicated constants files (ravendb/constants.py and ravendb/primitives/constants.py).
Fixed minor issues with class members names and imports.
Made sure tests related to streaming pass.

@poissoncorp
Copy link
Contributor

Please run black code formatter over the changes 🙏 it complains about it



# todo: https://issues.hibernatingrhinos.com/issue/RavenDB-20033
# class StreamCommand(RavenCommand[StreamResultResponse]):
Copy link
Contributor

Choose a reason for hiding this comment

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

should we uncomment/use it somewhere?

Copy link
Author

Choose a reason for hiding this comment

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

Part of latest commit with document streaming

@@ -25,7 +25,7 @@


_T = TypeVar("_T")
_Operation_T = TypeVar("_Operation_T")
_Operation_Return_Type = TypeVar("_Operation_Return_Type")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_Operation_Return_Type = TypeVar("_Operation_Return_Type")
_T_Operation_Return = TypeVar("_T_Operation_Return")

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -136,8 +136,8 @@ def for_database(self, database_name: str) -> MaintenanceOperationExecutor:
return MaintenanceOperationExecutor(self._store, database_name)

def send(
self, operation: Union[VoidMaintenanceOperation, MaintenanceOperation[_Operation_T]]
) -> Optional[_Operation_T]:
self, operation: Union[VoidMaintenanceOperation, MaintenanceOperation[_Operation_Return_Type]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

_T in the beginning tells that's a type. Too bad I wasn't that smart to do that myself back then

Copy link
Author

Choose a reason for hiding this comment

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

done


result = stream_operation.set_result(command.result)

return self._yield_result(query_or_raw_query, result), stats
Copy link
Contributor

Choose a reason for hiding this comment

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

query_or_raw_query reminds me times when I've performed duck typing in methods instead of creating 2 separate ones.
Does it matter here, if it's raw or regular query, in terms of the data flow? Is there any if type== that splits the code?

Copy link
Author

Choose a reason for hiding this comment

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

Doesn's seem like there is any difference, as in the very first line of _yield_result, it uses members query._fields_to_fetch_token, query.is_project_into and query.invoke_after_stream_executed without ever checking for the type.

self.request_executor.execute_command(command, self.session_info)

result = stream_operation.set_result(command.result)
return DocumentSession._Advanced._StreamIterator(self, result, None, False, None, object_type)
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we make it more public, so we don't need to provide full static path here?

Copy link
Author

Choose a reason for hiding this comment

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

Created a property that returns it as a type, although I'm not entirely sure it's necessary.

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.

2 participants