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

fix(core): fix the remote column inferring and disable simplify expression rule #874

Merged
merged 5 commits into from
Nov 1, 2024

Conversation

goldmedal
Copy link
Contributor

@goldmedal goldmedal commented Oct 31, 2024

Description

This PR fixed multiple issues.

Disable SimplifyExpressionRule

This rule will simplify an expression to another expression if it can be evaluated. For example, current_date would be simplified to the real value like CAST('2024-10-31' DATE). However, we should execute this function on the DataSource side to make sure the time or timezone issue can be aligned.

Fix the Remote Column inferring

The remote column has some bugs. I missed to apply this way when generating the ModelSourceNode.

Fix the isHidden field

We always serialize the field name using camelCase style.

@github-actions github-actions bot added core ibis rust Pull requests that update Rust code python Pull requests that update Python code labels Oct 31, 2024
@goldmedal goldmedal changed the title fix(core) fix the remote column inferring and disable simplify expression rule fix(core): fix the remote column inferring and disable simplify expression rule Oct 31, 2024
@goldmedal goldmedal marked this pull request as ready for review October 31, 2024 10:24
Copy link
Contributor

@grieve54706 grieve54706 left a comment

Choose a reason for hiding this comment

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

LGTM

@grieve54706
Copy link
Contributor

@goldmedal Could you check the transformed SQL is changed for Postgres?

@goldmedal
Copy link
Contributor Author

@goldmedal Could you check the transformed SQL is changed for Postgres?

The planned SQL is

SELECT 
  orders."timestamp", 
  orders."timestamptz", 
  orders.order_cust_key, 
  orders.o_custkey, 
  orders.o_orderdate, 
  orders.o_orderkey, 
  orders.o_orderstatus, 
  orders.o_totalprice 
FROM 
  (
    SELECT 
      CAST(
        '2024-01-01T23:59:59' AS TIMESTAMP
      ) AS "timestamp", 
      CAST(
        '2024-01-01T23:59:59' AS TIMESTAMP WITH TIME ZONE
      ) AS "timestamptz", 
      concat(
        CAST(
          public.orders.o_orderkey AS VARCHAR
        ), 
        '_', 
        CAST(
          public.orders.o_custkey AS VARCHAR
        )
      ) AS order_cust_key, 
      public.orders.o_custkey AS o_custkey, 
      public.orders.o_orderdate AS o_orderdate, 
      public.orders.o_orderkey AS o_orderkey, 
      public.orders.o_orderstatus AS o_orderstatus, 
      public.orders.o_totalprice AS o_totalprice 
    FROM 
      public.orders
  ) AS orders 
LIMIT 
  1

The dialect SQL is

SELECT 
  orders."timestamp", 
  orders."timestamptz", 
  orders.order_cust_key, 
  orders.o_custkey, 
  orders.o_orderdate, 
  orders.o_orderkey, 
  orders.o_orderstatus, 
  orders.o_totalprice 
FROM 
  (
    SELECT 
      CAST(
        '2024-01-01T23:59:59' AS TIMESTAMP
      ) AS "timestamp", 
      CAST(
        '2024-01-01T23:59:59' AS TIMESTAMPTZ
      ) AS "timestamptz", 
      CONCAT(
        CAST(
          public.orders.o_orderkey AS VARCHAR
        ), 
        '_', 
        CAST(
          public.orders.o_custkey AS VARCHAR
        )
      ) AS order_cust_key, 
      public.orders.o_custkey AS o_custkey, 
      public.orders.o_orderdate AS o_orderdate, 
      public.orders.o_orderkey AS o_orderkey, 
      public.orders.o_orderstatus AS o_orderstatus, 
      public.orders.o_totalprice AS o_totalprice 
    FROM 
      public.orders
  ) AS orders 
LIMIT 
  1

It seems to generate TIMESTAMPTZ, so the result has a UTC suffix.

@grieve54706 grieve54706 merged commit 7e84d56 into Canner:main Nov 1, 2024
13 checks passed
@goldmedal goldmedal deleted the bugfix/fix-some-syntax branch November 4, 2024 08:44
@grieve54706 grieve54706 added this to the 0.11.3 milestone Nov 12, 2024
grieve54706 pushed a commit that referenced this pull request Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core ibis python Pull requests that update Python code rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants