Skip to content

Added asNumber to OrderByColumnSpec#940

Closed
flowbehappy wants to merge 2 commits intoapache:masterfrom
flowbehappy:master
Closed

Added asNumber to OrderByColumnSpec#940
flowbehappy wants to merge 2 commits intoapache:masterfrom
flowbehappy:master

Conversation

@flowbehappy
Copy link
Copy Markdown
Contributor

This commit make dimension can be sorted numerically(only "long" here) besides lexicographically. e.g

query:
...
"dimensions": [
    "id",
],
"limitSpec": {
    "type": "default",
    "limit": 20,
    "columns": [
        {
            "asNumber": true,
            "dimension": "id",
            "direction": "ASCENDING"
        }
    ]
},
...

result:
id  name    score
0   alias   90
1   bob     75
2   joe     60
11  flow    88
14  kevin   54

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please have an explicit type parameter here if at all possible, rather than Object

@nishantmonu51
Copy link
Copy Markdown
Member

It will be great, if you can add unit test for this feature.
you can have a look at GroupByQueryRunnerTest for existing groupBy unit tests.

@metamx-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

1 similar comment
@metamx-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@flowbehappy
Copy link
Copy Markdown
Contributor Author

@drcrallen @nishantmonu51 I added test case and did some code clean up. And sorry for taking so long.

@drcrallen
Copy link
Copy Markdown
Contributor

Thanks @flowbehappy few more comments:

If a dimension is called "id" I would expect it to be a high cardinality dimension rather than just "default". Is "id" supposed to be relatively high cardinality in this case?

@flowbehappy
Copy link
Copy Markdown
Contributor Author

@drcrallen Totally agree. I will generate another more real test data.

@fjy fjy force-pushed the master branch 2 times, most recently from 8b0ec82 to d05032b Compare February 1, 2015 04:57
@fjy
Copy link
Copy Markdown
Contributor

fjy commented Mar 2, 2015

@cheddar @himanshug @xvrl any thoughts here? My biggest high level concern is that it only Longs are supported, but it is a useful feature.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It does not seem too hard to make this feature complete by adding support for doubles as well.
We can use a regex here instead to detect if its a double value or long value or something else. Accordingly use Double.parseDouble(..) or Long.parseLong(..) .
And in the compare above add another case to handle Doubles.
Also, you can refactor this code a little bit by having a private method like
private Comparable getDimensionValue() {

}
and calling return getDimensionValue(dimList.get(0));

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Aug 30, 2016

I believe this use case is addressed by #3270 through "dimensionOrder": "numeric".

@gianm gianm closed this Aug 30, 2016
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.

7 participants