-
Notifications
You must be signed in to change notification settings - Fork 395
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
Add DENSE_RANK aggregate function #1484
Add DENSE_RANK aggregate function #1484
Conversation
Implementation and unit tests.
data/org.eclipse.birt.data.aggregation/.settings/org.eclipse.jdt.core.prefs
Outdated
Show resolved
Hide resolved
Moved back to JavaSE-11
For future reference, you can modify the Bundle-RequiredExecutionEnvironment in the MANIFEST.MF and save. Then on the context menu of the plug-in project use Plug-in Tools -> Update Classpath... which will update the .classpath and the org.eclipse.jdt.core.prefs to match that value. |
@hvbtup I suspect that you have a lot on your plate. Still, I would like you to test this out since you suggested this new aggregation function and have a lot more knowledge about report design than me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works well if the expression is NOT NULL.
But the report fails with
java.lang.ClassCastException: class org.eclipse.birt.data.aggregation.impl.rank.NullObject cannot be cast to class java.lang.Comparable
if the expression can be null.
For example, the report crashes if I use the following query
select
PRODUCTCODE
, PRODUCTNAME
, PRODUCTLINE
, PRODUCTSCALE
, PRODUCTVENDOR
, PRODUCTDESCRIPTION
, QUANTITYINSTOCK
, BUYPRICE
, MSRP
, case when productname like '%f%' then null else buyprice end as x
from products
and then use RANK or DENSERANK based on the column X.
A minor issue (but my German picky eyes stumble over it...) is the non-consistent spelling.
There are
- DENSERANK (the visible form, which is OK, because it is in line with eg. RUNNINGCOUNT)
- TOTAL_DENSE__RANK_FUNC (with two underscores before RANK)
- DENSE_RANK
Fixed NPE and did some cleaning
Fixed name inconsistency of DENSERANK
With this latest commit everything should be addressed. I did some more clean-up to the RankObjComparator, added generic types to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking and working much better now!
I think it's worth noting in the docs (if we only had good docs) how the BIRT aggregate functions RANK and DENSERANK treat NULL values:
They are considered to be less than all NON NULL values.
Thus with ascending order, NULLs come first, and with descending order, NULLs come last.
This behavior is the same as with the RANK aggregate in previous releases, that's good.
People with a strong DB-specific background (like me) may be surprised, though.
In Oracle, ORDER BY expr [ASC|DESC] NULLS LAST will put NULLs at the end independent of ascending or descending ordering. This SQL syntax is standard, but the default is vendor-specific. In Oracle, NULLS LAST is the default; to get the nulls first one has to specify NULLs FIRST explicitly.
However, this is vendor-dependent (see https://modern-sql.com/concept/null).
We might add explicit control over NULL values later.
A workaround (in SQL) is shown on the modern-sql page.
That principle can be used with BIRT as well by adding a boolean expression that tells us if the column is null or not; then we can first sort by this column and then by the column value itself.
Today there are no way to pass parameters down to Aggregator functions / Accumulators. If we were to make that possible it would benefit #1089 as well. I might look into that one as my next thing |
Implementation and unit tests of DENSE_RANK. I could not decide if I should change the file format version or not, I ended up not doing it.