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

Document template fixes #1894

Merged
merged 17 commits into from
Dec 17, 2024
Merged

Document template fixes #1894

merged 17 commits into from
Dec 17, 2024

Conversation

ricopinazo
Copy link
Contributor

@ricopinazo ricopinazo commented Dec 10, 2024

What changes were proposed in this pull request?

  • Add default document templates
  • Update vector API (on the server as well) to allow choosing between using the default template, a custom one, or nothing at all, for each of the three types of entities
  • Fix a bug causing subgraphs to allow containing the same node more than once
  • Fix a bug causing NaN float to panic when querying through GraphQL
  • Review public API to stick to temporal_props / constant_props naming convention

Why are the changes needed?

Having default templates is a first step towards a smart search view on the generic UI

Does this PR introduce any user-facing change? If yes is this documented?

Yes, and the docstrings were updated accordingly

How was this patch tested?

  • New tests were added to test the default templates in rust
  • A new python test was added to double-check the default template is used if the user doesn't specify anything

Are there any further changes required?

No

@ricopinazo ricopinazo marked this pull request as ready for review December 16, 2024 13:43
raphtory/src/db/graph/views/node_subgraph.rs Show resolved Hide resolved
@@ -201,7 +204,10 @@ impl DocumentTemplate {
let mut env = Environment::new();
let template = build_template(&mut env, template);
match template.render(NodeTemplateContext::from(node)) {
Ok(document) => Box::new(std::iter::once(document.into())),
Ok(mut document) => {
truncate(&mut document);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mention this in the docs / try to make this configurable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add helper funtion node.generate_document() to allow debugging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ideally simply use multi documents for big documents

{% for (key, values) in temporal_properties|items %}
{{ key }}:
{% for (time, value) in values %}
- changed to {{ value }} at {{ time|datetimeformat }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

allow choosing between numbers or timestamps in the API

Copy link
Collaborator

@miratepuffin miratepuffin left a comment

Choose a reason for hiding this comment

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

Pair reviewed with Pedro - tickets being made for spotted issues, but LGTM for now

@ricopinazo ricopinazo merged commit f00e7a8 into master Dec 17, 2024
19 checks passed
@ricopinazo ricopinazo deleted the fix/document-template branch December 17, 2024 17:01
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