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

Custom Column does not maintain data type #13122

Closed
dacort opened this issue Aug 14, 2020 · 6 comments
Closed

Custom Column does not maintain data type #13122

dacort opened this issue Aug 14, 2020 · 6 comments
Assignees
Labels
Priority:P2 Average run of the mill bug Querying/Notebook Items specific to the Custom/Notebook query builder Querying/ .Reproduced Issues reproduced in test (usually Cypress) Type:Bug Product defects
Milestone

Comments

@dacort
Copy link
Contributor

dacort commented Aug 14, 2020

Describe the bug
When using the coalesce (or case) custom expressions, the resulting value does not maintain the data type of the original values. This means that I can't effectively use a filter on the subsequent values.

Logs
n/a

To Reproduce
Steps to reproduce the behavior:

  1. Go to question 1441 in our internal stats instance
  2. Try to filter the Issue Closed At column
  3. Notice that you get a filter box for numeric values

Expected behavior
I would expect the type of the original columns to be maintained.

Screenshots
image

Information about your Metabase Installation:

{
  "browser-info": {
    "language": "en-US",
    "platform": "MacIntel",
    "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:79.0) Gecko/20100101 Firefox/79.0",
    "vendor": ""
  },
  "system-info": {
    "file.encoding": "UTF-8",
    "java.runtime.name": "OpenJDK Runtime Environment",
    "java.runtime.version": "11.0.8+10",
    "java.vendor": "AdoptOpenJDK",
    "java.vendor.url": "https://adoptopenjdk.net/",
    "java.version": "11.0.8",
    "java.vm.name": "OpenJDK 64-Bit Server VM",
    "java.vm.version": "11.0.8+10",
    "os.name": "Linux",
    "os.version": "4.14.186-110.268.amzn1.x86_64",
    "user.language": "en",
    "user.timezone": "GMT"
  },
  "metabase-info": {
    "databases": [
      "googleanalytics",
      "mysql",
      "druid",
      "postgres",
      "bigquery",
      "redshift",
      "mongo",
      "h2"
    ],
    "hosting-env": "unknown",
    "application-database": "postgres",
    "application-database-details": {
      "database": {
        "name": "PostgreSQL",
        "version": "9.6.11"
      },
      "jdbc-driver": {
        "name": "PostgreSQL JDBC Driver",
        "version": "42.2.8"
      }
    },
    "run-mode": "prod",
    "version": {
      "date": "2020-08-13",
      "tag": "v0.37.0-snapshot",
      "branch": "master",
      "hash": "81e1e87"
    },
    "settings": {
      "report-timezone": "US/Pacific"
    }
  }
}

Severity
Annoying - it prevents me from effectively using coalesce where I want to filter the resulting value.

Additional context
Surprisingly, the formatting of the column seems to be maintained. 🤔
image

@dacort dacort added Type:Bug Product defects .Needs Triage labels Aug 14, 2020
@flamber flamber added Priority:P2 Average run of the mill bug Querying/ Querying/Notebook Items specific to the Custom/Notebook query builder and removed .Needs Triage labels Aug 15, 2020
@flamber
Copy link
Contributor

flamber commented Aug 15, 2020

Generic reproduce:

  1. Custom question > Sample Dataset > Orders
  2. Custom Column: case([Discount] > 0, [Created At], [Product → Created At]) as "Test"
  3. Filter "Test" only allows numeric inputs

Related #12938

@ariya
Copy link
Contributor

ariya commented Feb 15, 2021

Triaging investigation, for the sake of understanding the problem.

When a filter is constructed with a field from the table (i.e. it's not a custom column), for instance Created At for the Previous 30 Days, the query (in MBQL) looks like this:

["time-interval",["field-id",12],-30,"day",{}]

With this filter, the FilterPopoverPicker will create a DatePicker component to let the use choose the time interval:

image

However, when a filter is constructed from a custom column, the query looks radically different (datetest is the name of the custom column):

["=",["expression","datetest"],null]

which will lead FilterPopoverPicker to create a (rather generic) FieldValuesWidget which doesn't work with date/time field:

image

In the latter case, the corresponding field of the dimension of the filter is as follows:

image

Note that type/Float type, which obviously leads to the (incorrect) choice of rendering a FieldValuesWidget component.

@ariya ariya changed the title coalesce Custom Column does not maintain data type Custom Column does not maintain data type Feb 15, 2021
@nemanjaglumac nemanjaglumac added this to File Info in Cypress Testing Feb 16, 2021
@nemanjaglumac nemanjaglumac moved this from File Info to Backlog in Cypress Testing Feb 16, 2021
@ariya
Copy link
Contributor

ariya commented Feb 16, 2021

So finally I found out where this field type is hardcoded: it's inside (surprise) ExpressionDimension, more precisely on its field() method:

  field() {
    return new Field({
      id: this.mbql(),
      name: this.name(),
      display_name: this.displayName(),
      semantic_type: null,
      base_type: "type/Float",
      // HACK: need to thread the query through to this fake Field
      query: this._query,
      table: this._query ? this._query.table() : null,
    });
  }

In order to avoid hardcoding base_type to type/Float, ExpressionDimension needs to infer the return type from the expression (in this bug, either case or coalesce, but generally could be any custom expression).

Currently the type checker work for v39 (#14214) isn't yet bi-directional. I'm still contemplating whether I can fit it the remaining activity to close the gap between the current type-checker and something like Hindley-Milner type-system reasonably within the timeline of v39 milestone.

@camsaul
Copy link
Member

camsaul commented Mar 19, 2021

Determining the type of the expression is something we should do in the shared lib I think. That information would be good to have on the backend too -- for query validation and to make sure we determine the correct type information for things like nested queries

@ariya
Copy link
Contributor

ariya commented May 20, 2021

This should be fixed with the work in #15952.

This was referenced May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:P2 Average run of the mill bug Querying/Notebook Items specific to the Custom/Notebook query builder Querying/ .Reproduced Issues reproduced in test (usually Cypress) Type:Bug Product defects
Projects
Development

No branches or pull requests

5 participants