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

Properly display computed axis on charts, use local time for all datetime functions #1116

Merged
merged 11 commits into from
Jul 26, 2020

Conversation

sc1f
Copy link
Contributor

@sc1f sc1f commented Jul 9, 2020

This PR makes several fixes to computed functions and highcharts/d3fc:

  • Fixes an issue where computed columns would not be displayed correctly in chart axis, both in highcharts and d3fc. This was due to computed columns no longer being present in the table schema.
  • Fixes an issue where datetime computed functions calculated buckets in UTC and not local time. This caused issues with datetimes that were part of one bucket in local time, but another in UTC - i.e. 03/15/2020 23:59 EST becomes 03/16/2020 04:59 UTC.
  • Distinguishes between computed_schema on the view and on the table in PerspectiveManager, as they have different signatures. A future improvement would be renaming Table.computed_schema to something like Table.get_computed_schema, as the table method receives a set of computed columns and calculates their output types without actually creating the columns themselves.
  • Cleans up the internals of PerspectiveManager and returns an error message if table.delete() is called on a remote table. Previously, table.delete() would fail due to code flow reasons, which prevented the method from being called but is opaque. This provides a transparent error message and explanation.
  • Fixes the last by index aggregate to use psp_okey instead of psp_pkey in the aggspec, as psp_pkey does not exist in some of the intermediate data structures used during a live update. Additionally, rename AGGTYPE_LAST to AGGTYPE_LAST_BY_INDEX, as the last aggregate uses AGGTYPE_LAST_VALUE.

@sc1f sc1f added bug Concrete, reproducible bugs C++ JS Python labels Jul 9, 2020
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -369,7 +369,7 @@ jobs:
- script: npm install -g yarn
displayName: "Install Yarn"

- script: yarn
- script: yarn --network-timeout 600000
Copy link
Member

Choose a reason for hiding this comment

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

??

@@ -48,6 +48,8 @@ function treemap(container, settings) {
.each(function({split, data}) {
const treemapSvg = d3.select(this);

console.log(split, data);
Copy link
Member

Choose a reason for hiding this comment

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

Debug leak

@texodus
Copy link
Member

texodus commented Jul 26, 2020

Thanks for the PR! This won't be the last time we address this aggregate type, due ot the large body of work necessary to properly integrate this with sort, but its a great start!

@texodus texodus merged commit c536028 into master Jul 26, 2020
@texodus texodus deleted the fix-computed-charts branch July 26, 2020 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Concrete, reproducible bugs C++ JS Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants