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

executor: support "show columns from" for view #8863

Merged
merged 5 commits into from Jan 5, 2019

Conversation

5 participants
@AndrewDi
Copy link
Contributor

AndrewDi commented Dec 28, 2018

What problem does this PR solve?

Modify DESCRIBE TABLE to describe view columns

What is changed and how it works?

ref proposal Proposal: Implement View

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change
    Export function func (b *PlanBuilder) BuildDataSourceFromView(dbName model.CIStr, tableInfo *model.TableInfo)

Side effects
none


This change is Reviewable

@AndrewDi AndrewDi referenced this pull request Dec 28, 2018

Open

Tracking view feature implement #8520

12 of 17 tasks complete

@zz-jason zz-jason added this to In progress in Support View via automation Dec 29, 2018

@AndrewDi

This comment has been minimized.

Copy link
Contributor

AndrewDi commented Jan 2, 2019

@XuHuaiyu
Copy link
Contributor

XuHuaiyu left a comment

Any test for this?

@AndrewDi

This comment has been minimized.

Copy link
Contributor

AndrewDi commented Jan 3, 2019

@XuHuaiyu show_test.go do not contain any DESCRIBE TBALE test, so I didn't add test either.

@XuHuaiyu

This comment has been minimized.

Copy link
Contributor

XuHuaiyu commented Jan 3, 2019

We'd better add some test cases for it.

@lamxTyler
Copy link
Member

lamxTyler left a comment

LGTM

@AndrewDi

This comment has been minimized.

Copy link
Contributor

AndrewDi commented Jan 4, 2019

/run-all-tests

@AndrewDi

This comment has been minimized.

Copy link
Contributor

AndrewDi commented Jan 4, 2019

@XuHuaiyu PTAL

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 4, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@081a2c5). Click here to learn what that means.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #8863   +/-   ##
=========================================
  Coverage          ?   67.54%           
=========================================
  Files             ?      363           
  Lines             ?    75111           
  Branches          ?        0           
=========================================
  Hits              ?    50731           
  Misses            ?    19907           
  Partials          ?     4473
Impacted Files Coverage Δ
planner/core/logical_plan_builder.go 74.02% <100%> (ø)
executor/show.go 41.47% <80%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 081a2c5...7abbb96. Read the comment docs.

for _, col := range cols {
viewColumn := viewSchema.FindColumnByName(col.Name.L)
if viewColumn != nil {
col.FieldType = *viewColumn.GetType()

This comment has been minimized.

@zz-jason

zz-jason Jan 5, 2019

Member

We can't get the column type from the columns returned by tb.Cols()?

This comment has been minimized.

@AndrewDi

AndrewDi Jan 5, 2019

Contributor

Yes, view's column tableinfo only store column name, so we need to extract sql's schema.column to get column type.

This comment has been minimized.

@zz-jason

zz-jason Jan 5, 2019

Member

Can we fill the column type info when this table info is generated?

This comment has been minimized.

@AndrewDi

AndrewDi Jan 5, 2019

Contributor

I don't think so, because undertable's column type and column length can be change.

This comment has been minimized.

@zz-jason

zz-jason Jan 5, 2019

Member
MySQL(root@localhost:test) > create table t(a bigint, b bigint);
Query OK, 0 rows affected (0.04 sec)

MySQL(root@localhost:test) > create view v as select * from t;
Query OK, 0 rows affected (0.07 sec)

MySQL(root@localhost:test) > show columns from v;
+-------+------------+------+-----+---------+-------+
| Field | Type       | Null | Key | Default | Extra |
+-------+------------+------+-----+---------+-------+
| a     | bigint(20) | YES  |     | NULL    |       |
| b     | bigint(20) | YES  |     | NULL    |       |
+-------+------------+------+-----+---------+-------+
2 rows in set (0.01 sec)

MySQL(root@localhost:test) > drop table t;
Query OK, 0 rows affected (0.03 sec)

MySQL(root@localhost:test) > create table t(a double, b double, c double);
Query OK, 0 rows affected (0.07 sec)

MySQL(root@localhost:test) > show columns from v;
+-------+--------+------+-----+---------+-------+
| Field | Type   | Null | Key | Default | Extra |
+-------+--------+------+-----+---------+-------+
| a     | double | YES  |     | NULL    |       |
| b     | double | YES  |     | NULL    |       |
+-------+--------+------+-----+---------+-------+
2 rows in set (0.01 sec)

Got it. Confirmed in MySQL.

This comment has been minimized.

@zz-jason

zz-jason Jan 5, 2019

Member

@AndrewDi Could you add a comment to specify why we need to build logical plan on the view to get the column types?

This comment has been minimized.

@AndrewDi

AndrewDi Jan 5, 2019

Contributor

Done

@AndrewDi AndrewDi force-pushed the AndrewDi:view/describe_table branch from 2e8077a to 7abbb96 Jan 5, 2019

Support View automation moved this from In progress to Reviewer approved Jan 5, 2019

@zz-jason
Copy link
Member

zz-jason left a comment

LGTM

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Jan 5, 2019

/run-all-tests

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Jan 5, 2019

@zz-jason zz-jason changed the title planner,executor: alter describe table to support view executor: support "show columns from" for view Jan 5, 2019

@zz-jason zz-jason merged commit 78a51a4 into pingcap:master Jan 5, 2019

12 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

Support View automation moved this from Reviewer approved to Done Jan 5, 2019

@AndrewDi AndrewDi deleted the AndrewDi:view/describe_table branch Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment