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

update field name #354

Merged
merged 3 commits into from
Oct 12, 2015
Merged

update field name #354

merged 3 commits into from
Oct 12, 2015

Conversation

siddontang
Copy link
Member

Use as name explicitly. Old code uses as name for field name, if has not, uses origin expression string instead. e,g, select 1 as a, we will use "a" as the field name, select 1, we will use "1" as the field name.

This mechanism is not proper for later field name handing in group by, order by or other clauses. e.g, select t.c as "t.c" from t, using the old way we cannot know "t.c" is an alia name or just expression representation. So now the GetField* function in field package is complex and confused.

I think we will use AsName in field struct explicitly, we will check AsName to see whether a field has an alias name or not, if no alias name, we can try to use expression representation then.

@@ -144,12 +144,12 @@ func (*testResultFieldSuite) TestMain(c *C) {

// For GetFieldIndex
f1 := &field.Field{
Expr: &expression.Ident{CIStr: model.NewCIStr("c1")},
Name: "a",
Expr: &expression.Ident{CIStr: model.NewCIStr("c1")},
Copy link
Member

Choose a reason for hiding this comment

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

Why use '&' herer but does not use '&' in the above test file?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see any other place not using "&" for expression.Ident.

Copy link
Member

Choose a reason for hiding this comment

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

Value is kind of weird, Maybe we should change its function to use pointer.
Expr: expression.Value{Val: "c1+1"},

Copy link
Member

Choose a reason for hiding this comment

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

+1, all expressions use pointer is better.

@shenli
Copy link
Member

shenli commented Oct 12, 2015

LGTM

name = expr.String()
}
$$ = &field.Field{Expr: expr, Name: name}
$$ = &field.Field{Expr: expr, AsName: name}
Copy link
Member

Choose a reason for hiding this comment

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

How about

$$ = &field.Field{
     Expr: expression.Expr($1),
     AsName: $2.(string)
}

seems needless to define variables.

Copy link
Member Author

Choose a reason for hiding this comment

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

Em, seems no big different. :-)

@qiuyesuifeng
Copy link
Member

LGTM

siddontang added a commit that referenced this pull request Oct 12, 2015
@siddontang siddontang merged commit d594fd8 into master Oct 12, 2015
@siddontang siddontang deleted the siddontang/update-field-name branch October 12, 2015 13:49
YuJuncen pushed a commit to YuJuncen/tidb that referenced this pull request Apr 23, 2021
okJiang pushed a commit to okJiang/tidb that referenced this pull request Oct 19, 2021
iosmanthus added a commit to iosmanthus/tidb that referenced this pull request May 11, 2023
rleungx pushed a commit to rleungx/tidb that referenced this pull request Feb 26, 2024
…gcap#536)

* fix tiflash rules constraints by introducing `TiFlashConstraints` in config (pingcap#354)

Signed-off-by: iosmanthus <[email protected]>

* introduce tiflash replica min count (pingcap#371)

Co-authored-by: zzm <[email protected]>
Signed-off-by: iosmanthus <[email protected]>

* auto fix missing tiflash placment rules (pingcap#364)

Signed-off-by: iosmanthus <[email protected]>

* fix noop for setting tiflash replicas to 0

Signed-off-by: iosmanthus <[email protected]>

---------

Signed-off-by: iosmanthus <[email protected]>
Co-authored-by: zzm <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants