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

docs: Refresh view query language section #144

Merged
merged 6 commits into from
Jan 16, 2025

Conversation

johanandren
Copy link
Member

@johanandren johanandren commented Jan 13, 2025

@github-actions github-actions bot added the documentation documentation related label Jan 13, 2025
@johanandren johanandren marked this pull request as ready for review January 14, 2025 13:25
@johanandren
Copy link
Member Author

@pvlugter any more feature I have missed that would be good to cover here?

# Conflicts:
#	akka-javasdk-tests/src/test/java/akkajavasdk/ViewIntegrationTest.java
#	akka-javasdk-tests/src/test/java/akkajavasdk/components/views/AllTheTypesView.java
Copy link
Member

@octonato octonato left a comment

Choose a reason for hiding this comment

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

LGTM, but waiting Peter's feedback before merging.

Left a command about word 'support', but not blocking.


==== Grouping

Grouping of results based on a field is supported using `collect(*)`. Each found key leads to one returned entry, where
Copy link
Member

Choose a reason for hiding this comment

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

so, there was this other PR recently that replaces the usage of the term 'support' by alternatives.
#137

Suggested change
Grouping of results based on a field is supported using `collect(*)`. Each found key leads to one returned entry, where
Grouping of results based on a field is done using `collect(*)`. Each found key leads to one returned entry, where

But I see that we use the term quite a lot in this page. Hard to replace it in most cases, IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's all over this page, let's leave it for now.

@johanandren johanandren force-pushed the wip-view-docs-updates branch from 4929ef2 to 57c6f21 Compare January 14, 2025 16:18
Copy link
Contributor

@pvlugter pvlugter left a comment

Choose a reason for hiding this comment

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

LGTM. I think most things are covered now, and things that were Yugabyte limitations before already removed.

docs/src/modules/java/partials/query-syntax-reference.adoc Outdated Show resolved Hide resolved

==== Grouping

Grouping of results based on a field is supported using `collect(*)`. Each found key leads to one returned entry, where
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to also talk about 'structural' projections here? Where collect(*) uses all the fields, but you can also select/rename fields. Or nested collects. There are some examples under advanced queries, but could be useful with a reference description here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to cover 👍

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 in 98746c2

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 But doesn't yet replace the * in collect(*) with multiple selected fields. But maybe that should be left for the advanced queries anyway. Something you may want to do when there's more complex state and multiple queries for a view, and some of them want to only return the relevant subset of the state.


==== Count

Counting results matching a query can be done using `count(*)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably do want to use count(*) here, even if it's not a proper aggregation but a separate query for the total_count() for paging. But does this create a warning at the moment? Or maybe we can distinguish this usage from the other one (when it's combined with regular results) and only have the deprecation then.

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 didn't see any when testing a query like SELECT count(*) FROM somewhere but I'll double check if there is a warning in the log.

I'm showing off total_count() in the paging section, I tried to read up on issues, but I'm not entirely if I understood correctly wrt the two different count aggregations.

Copy link
Member Author

Choose a reason for hiding this comment

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

It indeed shows up right now, so would need some fix in the runtime for specific count queries:

[info] 2025-01-15 09:28:19,054 WARN  akka.javasdk.ServiceLog - Warning reported from Akka runtime: AK-00131 View [all_the_field_types_view] query method on [countRows]: The COUNT(*) operator for paged view queries is deprecated, replaced by the total_count() function.
[info] Update this view query to use `total_count() AS fieldName` instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

@johanandren johanandren merged commit d28fb66 into java-spi Jan 16, 2025
24 checks passed
@johanandren johanandren deleted the wip-view-docs-updates branch January 16, 2025 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation documentation related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants