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

Picking a date column for sorting results in "day" granularity sort order #2531

Closed
tlrobinson opened this issue May 4, 2016 · 8 comments
Closed
Assignees
Labels
.Correctness Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Querying/Processor Type:Bug Product defects
Milestone

Comments

@tlrobinson
Copy link
Contributor

When selecting a date column in the sort picker we incorrectly include "day" as the unit, causing rows to be sorted by day but sometimes not sorted correctly within each day.

Note that clicking a table header doesn't cause the unit to be included, so it sorts correctly. This can be used as a workaround until we fix this issue.

@agilliland agilliland added this to the 0.17.1 milestone May 5, 2016
@tlrobinson tlrobinson self-assigned this May 5, 2016
@agilliland
Copy link
Contributor

@tlrobinson can you explain a bit more how you are reproducing this? I'm not seeing this happen.

@tlrobinson
Copy link
Contributor Author

Incorrect, when using the sort picker:

screenshot 2016-05-06 10 36 44

Correct, when clicking a column header:

screenshot 2016-05-06 10 39 04

@agilliland
Copy link
Contributor

So, this is ultimately an issue on the backend where running a :rows query with a simple order-by clause on a datetime field is incorrectly forcing that field representation to be grouped by day.

https://github.com/metabase/metabase/blame/master/src/metabase/query_processor/resolve.clj#L106

For example when I run the above query I see the following, which is not the correct SQL because the order by clause should not be casting the created_at column to a date:

SELECT "PUBLIC"."ORDERS"."USER_ID" AS "USER_ID", "PUBLIC"."ORDERS"."TOTAL" AS "TOTAL", "PUBLIC"."ORDERS"."TAX" AS "TAX", "PUBLIC"."ORDERS"."SUBTOTAL" AS "SUBTOTAL", "PUBLIC"."ORDERS"."PRODUCT_ID" AS "PRODUCT_ID", "PUBLIC"."ORDERS"."ID" AS "ID", "PUBLIC"."ORDERS"."CREATED_AT" AS "CREATED_AT"
FROM "PUBLIC"."ORDERS"
ORDER BY CAST("PUBLIC"."ORDERS"."CREATED_AT" AS DATE) ASC
LIMIT 2000

@tlrobinson
Copy link
Contributor Author

@agilliland So I think we agreed the correct fix is:

  1. ensure the frontend uses a bare field-id rather than datetime_field with a granularity
  2. ensure the backend doesn't group by day when sorting by a datetime column without a datetime_field and granularity

I can handle 1, do you or @camsaul want to take 2?

@salsakran
Copy link
Contributor

@camsaul raw rows should definitely not cast the timestamp to a date. This is incorrect, and should be fixed.

We can and should default to timezone when charting aggregates by a timestamp, but in this case it is incorrect.

@camsaul
Copy link
Member

camsaul commented May 9, 2016

Ok @tlrobinson part 2 is merged so I'm going to assign this back to you so you can tackle part 1.

@camsaul camsaul assigned tlrobinson and unassigned camsaul May 9, 2016
@salsakran salsakran reopened this May 11, 2016
@salsakran salsakran added the Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness label May 11, 2016
@salsakran
Copy link
Contributor

Re-opening as the fix for this wasn't correct.

The resulting behavior is incorrect and a hard blocker on 0.17.1

@camsaul
Copy link
Member

camsaul commented May 12, 2016

Fixed by #2586

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Correctness Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness Querying/Processor Type:Bug Product defects
Projects
None yet
Development

No branches or pull requests

4 participants