You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2020/10/22 03:16:17 UTC
[GitHub] [shardingsphere] jingshanglu opened a new issue #7880: Add UT for table and expr.
jingshanglu opened a new issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880
## Background
Now, table and expr in statement are improved, so origin UT for it is not suitable. We need to add UT for table and expr.
parser UT entry:`shardingsphere/shardingsphere-sql-parser/shardingsphere-sql-parser-test/src/main/java/org/apache/shardingsphere/test/sql/parser/parameterized/engine/SQLParserParameterizedTest.java->assertSupportedSQL()`
## Solutions
1. Think about design of xxxAssert.java.
2. change expected result.(path:`shardingsphere/shardingsphere-sql-parser/shardingsphere-sql-parser-test/src/main/resources/case`)
3. The test class runs successfully(`shardingsphere/shardingsphere-sql-parser/shardingsphere-sql-parser-test/src/main/java/org/apache/shardingsphere/test/sql/parser/parameterized/engine/SQLParserParameterizedTest.java).
### eg
* #7872
## Note
We can discuss the reasonableness of the design and then implement it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev commented on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev commented on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-738481874
@jingshanglu Hi, I have redesigned the `expr` assertion, and then I will add its implementation, add more test cases and readjust the old test cases.
I hope you can review the main part of the design before proceeding to the next work.
- **ExpectedExpression**
All sub-expressions will be in the first level. It's make `expr` more conducive to reuse.
```java
public class ExpectedExpression extends AbstractExpectedSQLSegment {
@XmlElement(name = "between-expression")
private ExpectedBetweenExpression betweenExpression;
@XmlElement(name = "binary-operation-expression")
private ExpectedBinaryOperationExpression binaryOperationExpression;
@XmlElement(name = "column")
private ExpectedColumn column;
@XmlElement(name = "common-expression")
private ExpectedCommonExpression commonExpression;
@XmlElement(name = "exists-subquery")
private ExpectedExistsSubquery existsSubquery;
@XmlElement(name = "expression-projection")
private ExpectedExpressionProjection expressionProjection;
@XmlElement(name = "in-expression")
private ExpectedInExpression inExpression;
@XmlElement(name = "list-expression")
private ExpectedListExpression listExpression;
@XmlElement(name = "literal-expression")
private ExpectedLiteralExpression literalExpression;
@XmlElement(name = "not-expression")
private ExpectedNotExpression notExpression;
@XmlElement(name = "parameter-marker-expression")
private ExpectedParameterMarkerExpression parameterMarkerExpression;
@XmlElement(name = "subquery")
private ExpectedSubquery subquery;
}
```
- **OtherExpression**
Only `ExpectedBinaryOperationExpression` is displayed here, and others are similar to it.
```java
public class ExpectedBinaryOperationExpression extends AbstractExpectedSQLSegment {
@XmlElement(name = "left")
private ExpectedExpression left;
@XmlElement(name = "operator")
private ExpectedOperator operator;
@XmlElement(name = "right")
private ExpectedExpression right;
}
```
- **Test Case Example**
This is an example that includes all types of sub-expressions.
- `SQL`
```sql
SELECT * FROM t_order
WHERE (SELECT order_id FROM t_order_item WHERE status > ?)
OR EXISTS (SELECT order_id FROM t_order_item)
OR order_id ->"$[1]"
OR NOT order_id IN (1, 2, 3)
OR order_id BETWEEN 1 AND 3
OR order_id + INTERVAL 1 SECOND;
```
- `Test Case`
```xml
<select>
<from start-index="14" stop-index="20">
<simple-table name="t_order" start-index="14" stop-index="20" />
</from>
<projections distinct-row="false" start-index="7" stop-index="7">
<shorthand-projection start-index="7" stop-index="7" />
</projections>
<where start-index="22" stop-index="235">
<expr start-index="28" stop-index="235">
<binary-operation-expression start-index="28" stop-index="235">
<left start-index="28" stop-index="203">
<binary-operation-expression start-index="28" stop-index="203">
<left start-index="28" stop-index="175">
<binary-operation-expression start-index="28" stop-index="175">
<left start-index="28" stop-index="146">
<binary-operation-expression start-index="28" stop-index="146">
<left start-index="28" stop-index="125">
<binary-operation-expression start-index="28" stop-index="125">
<left start-index="28" stop-index="79">
<subquery start-index="28" stop-index="79">
<select>
<from start-index="50" stop-index="61">
<simple-table name="t_order_item" start-index="50" stop-index="61" />
</from>
<projections distinct-row="false" start-index="36" stop-index="43">
<column-projection name="order_id" start-index="36" stop-index="43" />
</projections>
<where start-index="63" stop-index="78">
<expr start-index="69" stop-index="78">
<binary-operation-expression start-index="69" stop-index="78">
<left start-index="69" stop-index="74">
<column name="status" start-index="69" stop-index="74" />
</left>
<operator type=">" />
<right start-index="78" stop-index="78">
<parameter-marker-expression value="0" start-index="78" stop-index="78" />
</right>
</binary-operation-expression>
</expr>
</where>
</select>
</subquery>
</left>
<operator type="OR" />
<right start-index="84" stop-index="125">
<exists-subquery start-index="84" stop-index="125">
<not>false</not>
<subquery start-index="91" stop-index="125">
<select>
<from start-index="113" stop-index="124">
<simple-table name="t_order_item" start-index="113" stop-index="124" />
</from>
<projections distinct-row="false" start-index="99" stop-index="106">
<column-projection name="order_id" start-index="99" stop-index="106" />
</projections>
</select>
</subquery>
</exists-subquery>
</right>
</binary-operation-expression>
</left>
<operator type="OR" />
<right start-index="130" stop-index="146">
<common-expression text="order_id ->"$[1]"" start-index="130" stop-index="146" />
</right>
</binary-operation-expression>
</left>
<operator type="OR" />
<right start-index="151" stop-index="175">
<in-expression start-index="151" stop-index="175">
<left start-index="151" stop-index="162">
<not-expression start-index="151" stop-index="162">
<expr start-index="155" stop-index="162">
<column name="order_id" start-index="155" stop-index="162" />
</expr>
</not-expression>
</left>
<not>false</not>
<right start-index="167" stop-index="175">
<list-expression start-index="167" stop-index="175">
<items start-index="168" stop-index="168">
<literal-expression value="1" start-index="168" stop-index="168" />
</items>
<items start-index="171" stop-index="171">
<literal-expression value="2" start-index="171" stop-index="171" />
</items>
<items start-index="174" stop-index="174">
<literal-expression value="3" start-index="174" stop-index="174" />
</items>
</list-expression>
</right>
</in-expression>
</right>
</binary-operation-expression>
</left>
<operator type="OR" />
<right start-index="180" stop-index="203">
<between-expression start-index="180" stop-index="203">
<left start-index="180" stop-index="187">
<column name="order_id" start-index="180" stop-index="187" />
</left>
<between-expr start-index="197" stop-index="197">
<literal-expression value="1" start-index="197" stop-index="197" />
</between-expr>
<and-expr start-index="203" stop-index="203">
<literal-expression value="3" start-index="203" stop-index="203" />
</and-expr>
<not>false</not>
</between-expression>
</right>
</binary-operation-expression>
</left>
<operator type="OR" />
<right start-index="208" stop-index="235">
<binary-operation-expression start-index="208" stop-index="235">
<left start-index="208" stop-index="215">
<column name="order_id" start-index="208" stop-index="215" />
</left>
<operator type="+" />
<right start-index="219" stop-index="235">
<expression-projection start-index="219" stop-index="235">
<text>INTERVAL1SECOND</text>
</expression-projection>
</right>
</binary-operation-expression>
</right>
</binary-operation-expression>
</expr>
</where>
</select>
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev edited a comment on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev edited a comment on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-729342237
> > > @lwtdev Now, there is no UT for expr, are you interested in fix it? The relevant issue is #7880
> >
> >
> > @jingshanglu I don't known what should to do about the work `UT for expr`, Which ‘expr’ need to add test cases?
>
> @lwtdev There is `WhereSegment` in selectStatement like this:
>
> ```
> public abstract class SelectStatement extends AbstractSQLStatement implements DMLStatement {
>
> private ProjectionsSegment projections;
>
> private TableSegment from;
>
> private WhereSegment where;
>
> private GroupBySegment groupBy;
>
> private OrderBySegment orderBy;
> }
> // WhereSegment contain expr
> public final class WhereSegment implements SQLSegment {
>
> private final int startIndex;
>
> private final int stopIndex;
>
> private final ExpressionSegment expr;
> }
> ```
>
> But in our test framework, there is no assertion of expr, like this:
>
> ```
> public final class WhereClauseAssert {
>
> /**
> * Assert actual where segment is correct with expected where clause.
> *
> * @param assertContext assert context
> * @param actual actual where segment
> * @param expected expected where clause
> */
> public static void assertIs(final SQLCaseAssertContext assertContext, final WhereSegment actual, final ExpectedWhereClause expected) {
> // TODO support expr in where assert
> }
> }
> ```
>
> just like `assertLockClause()` that you added.
@jingshanglu OK, I think I get it.
Find `TODO` tag in the `shardingsphere-sql-parser-test` module. Then fix them?
I will try to fix them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] jingshanglu commented on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-734196151
@lwtdev Ok, you can start from TableAssert (for UpdateStatementAssert).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev commented on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev commented on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-734229434
> @lwtdev Ok, you can start from TableAssert (for UpdateStatementAssert).
@jingshanglu I readly complete this part that `add TableAssert (for UpdateStatementAssert) and OwnerAssit`.
Now I am pending on `Expr` assert. 😄
> Because of the redesign of the Expr domain model, we need to design reasonably UT for Expr, How do you think?
Yes, I think so. I'm trying to redesign `Expr` assert now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev edited a comment on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev edited a comment on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-729342237
> > > @lwtdev Now, there is no UT for expr, are you interested in fix it? The relevant issue is #7880
> >
> >
> > @jingshanglu I don't known what should to do about the work `UT for expr`, Which ‘expr’ need to add test cases?
>
> @lwtdev There is `WhereSegment` in selectStatement like this:
>
> ```
> public abstract class SelectStatement extends AbstractSQLStatement implements DMLStatement {
>
> private ProjectionsSegment projections;
>
> private TableSegment from;
>
> private WhereSegment where;
>
> private GroupBySegment groupBy;
>
> private OrderBySegment orderBy;
> }
> // WhereSegment contain expr
> public final class WhereSegment implements SQLSegment {
>
> private final int startIndex;
>
> private final int stopIndex;
>
> private final ExpressionSegment expr;
> }
> ```
>
> But in our test framework, there is no assertion of expr, like this:
>
> ```
> public final class WhereClauseAssert {
>
> /**
> * Assert actual where segment is correct with expected where clause.
> *
> * @param assertContext assert context
> * @param actual actual where segment
> * @param expected expected where clause
> */
> public static void assertIs(final SQLCaseAssertContext assertContext, final WhereSegment actual, final ExpectedWhereClause expected) {
> // TODO support expr in where assert
> }
> }
> ```
>
> just like `assertLockClause()` that you added.
@jingshanglu OK, I think I get it.
Find `TODO` tag in the `shardingsphere-sql-parser-test` module. Then fix them?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] jingshanglu edited a comment on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
jingshanglu edited a comment on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-734194083
@lwtdev Now, we can do it step by step, `Expr` is more important, we can fix assert of `Expr` first,
- **Questions About** `ExpectedWhereClause`
1. Now , i think `ExpectedAndPredicate` and `ExpectedPredicate` in `ExpectedWhereClause` is unreasonable, maybe `ExpectedExpr` is better, and add some subclass for it, like `ExpectedInExpression`, `ExpectedNotExpression` and so on.
Because of the redesign of the `Expr` domain model, we need to design reasonably UT for `Expr`, How do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev edited a comment on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev edited a comment on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-733033027
@jingshanglu Hi, I'm trying to add TableAssert (for UpdateStatementAssert) OwnerAssit and WhereAssert in next pull request.
- **Questions About** `ExpectedWhereClause`
1. What's the different between `ExpectedAndPredicate` and `ExpectedPredicate` in `ExpectedWhereClause`?
2. Why does defined a `List` of `ExpectedAndPredicate` in `ExpectedWhereClause` , In which situation do we need more then one `ExpectedAndPredicate` in `ExpectedWhereClause`?
3. What does ` ExpectedPredicate` correspond to, is it `predicate`, or `booleanPrimary` , or `expr` ?
4. Why does `ExpectedPredicate` only had `column-left-value` for left part, what about `expr ` like `1 = t_order.status`?
- **Other TODO Tags**
I also found some `TODO` tags that not only need to add `*Assert` but also need to add parse logic. These changes have a lot of impact, so if we should fix them, I hope to fix it in other pull requests.
1. `AssignmentAssert`
```java
public static void assertIs(final SQLCaseAssertContext assertContext, final AssignmentSegment actual, final ExpectedAssignment expected) {
ColumnAssert.assertIs(assertContext, actual.getColumn(), expected.getColumn());
// TODO assert assign operator
AssignmentValueAssert.assertIs(assertContext, actual.getValue(), expected.getAssignmentValue());
}
```
Here need to add `operator` in `AssignmentSegment`, add change all test case that not contains `operator` before.
```g4
assignment
: columnName EQ_ assignmentValue
;
```
```java
public final class AssignmentSegment implements SQLSegment {
private final int startIndex;
private final int stopIndex;
private final ColumnSegment column;
private final ExpressionSegment value;
}
```
2 . `MySQLUseStatementAssert`
```java
public static void assertIs(final SQLCaseAssertContext assertContext, final MySQLUseStatement actual, final UseStatementTestCase expected) {
assertThat(assertContext.getText("Schema name assertion error: "), actual.getSchema(), is(expected.getSchema().getName()));
// TODO create a new assert class named `SchemaAssert`
// TODO extract and assert start index, stop index, start delimiter and end delimiter
}
```
Here need to change `String schema` to `SchemaSegment schema`. and add `visitSchemaSegmentContext` logic in visitor.
All code that references it also needs to be changed and tested.
```java
public final class MySQLUseStatement extends AbstractSQLStatement implements DALStatement, MySQLStatement {
private String schema;
}
```
3. `DropIndexStatementAssert` `AlterIndexStatementAssert` and `CreateIndexStatementAssert`
```java
private static void assertIndex(final SQLCaseAssertContext assertContext, final DropIndexStatement actual, final DropIndexStatementTestCase expected) {
// TODO should assert index for all databases(mysql and sqlserver do not parse index right now)
}
```
Here we also need add logic in parser.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev commented on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev commented on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-729342237
> > > @lwtdev Now, there is no UT for expr, are you interested in fix it? The relevant issue is #7880
> >
> >
> > @jingshanglu I don't known what should to do about the work `UT for expr`, Which ‘expr’ need to add test cases?
>
> @lwtdev There is `WhereSegment` in selectStatement like this:
>
> ```
> public abstract class SelectStatement extends AbstractSQLStatement implements DMLStatement {
>
> private ProjectionsSegment projections;
>
> private TableSegment from;
>
> private WhereSegment where;
>
> private GroupBySegment groupBy;
>
> private OrderBySegment orderBy;
> }
> // WhereSegment contain expr
> public final class WhereSegment implements SQLSegment {
>
> private final int startIndex;
>
> private final int stopIndex;
>
> private final ExpressionSegment expr;
> }
> ```
>
> But in our test framework, there is no assertion of expr, like this:
>
> ```
> public final class WhereClauseAssert {
>
> /**
> * Assert actual where segment is correct with expected where clause.
> *
> * @param assertContext assert context
> * @param actual actual where segment
> * @param expected expected where clause
> */
> public static void assertIs(final SQLCaseAssertContext assertContext, final WhereSegment actual, final ExpectedWhereClause expected) {
> // TODO support expr in where assert
> }
> }
> ```
>
> just like `assertLockClause()` that you added.
@jingshanglu OK, I think I get it.
I will find "TODO" in the "shardingsphere-sql-parser-test" module. Then fix them?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] wenweibin commented on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
wenweibin commented on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-723726152
> @wenweibin Yes, we also need to optimize other statement definitions. The commonly used ones are `alterTable`, `createTable`. We can start with these statements. I will create some subtasks according to the order of usage frequency. In #7791. Welcome to join the shardingsphere community
Ok，I will begin to work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev edited a comment on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev edited a comment on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-750300900
@jingshanglu Hi, I have completed the `expr` assertion of `ExpectedJoinTable`.
And I found that some other segments also contain `expr`, but no test cases
Do these need to add test cases?
- [ ] assignmentValue
- [ ] call
- [ ] doStatement
- [ ] projection
```g4
assignmentValue
: expr | DEFAULT | blobValue
;
call
: CALL identifier (LP_ (expr (COMMA_ expr)*)? RP_)?
;
doStatement
: DO expr (COMMA_ expr)*
;
projection
: expr (AS? alias)? | qualifiedShorthand
;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev edited a comment on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev edited a comment on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-734229434
> @lwtdev Ok, you can start from TableAssert (for UpdateStatementAssert).
@jingshanglu I readly completed a part which is `add TableAssert (for UpdateStatementAssert) and OwnerAssit`.
Now I am pending on `Expr` assert. 😄
> Because of the redesign of the Expr domain model, we need to design reasonably UT for Expr, How do you think?
Yes, I think so. I'm trying to redesign `Expr` assert now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev commented on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev commented on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-733033027
@jingshanglu Hi, I'm trying to add TableAssert (for UpdateStatementAssert) OwnerAssit and WhereAssert in next pull request.
- **Questions About** `ExpectedWhereClause`
1. What's the different between `ExpectedAndPredicate` and `ExpectedPredicate` in `ExpectedWhereClause`?
2. Why does define a `List` of `ExpectedAndPredicate` in `ExpectedPredicate` , In which situation do we need more then one `ExpectedAndPredicate` in `ExpectedWhereClause`?
- **Other TODO Tags**
I also found some `TODO` tags that need not only add `*Assert` but should need to add parse logic. These changes have a lot of impact, so if we should fix them, I hope to fix it in other pull requests.
1. `AssignmentAssert`
```java
public static void assertIs(final SQLCaseAssertContext assertContext, final AssignmentSegment actual, final ExpectedAssignment expected) {
ColumnAssert.assertIs(assertContext, actual.getColumn(), expected.getColumn());
// TODO assert assign operator
AssignmentValueAssert.assertIs(assertContext, actual.getValue(), expected.getAssignmentValue());
}
```
Here need to add `operator` in `AssignmentSegment`, add change all test case that not contains `operator` before.
```g4
assignment
: columnName EQ_ assignmentValue
;
```
```java
public final class AssignmentSegment implements SQLSegment {
private final int startIndex;
private final int stopIndex;
private final ColumnSegment column;
private final ExpressionSegment value;
}
```
2 . `MySQLUseStatementAssert`
```java
public static void assertIs(final SQLCaseAssertContext assertContext, final MySQLUseStatement actual, final UseStatementTestCase expected) {
assertThat(assertContext.getText("Schema name assertion error: "), actual.getSchema(), is(expected.getSchema().getName()));
// TODO create a new assert class named `SchemaAssert`
// TODO extract and assert start index, stop index, start delimiter and end delimiter
}
```
Here need to change `String schema` to `SchemaSegment schema`. and add `visitSchemaSegmentContext` logic in visitor.
All code that references it also needs to be changed and tested.
```java
public final class MySQLUseStatement extends AbstractSQLStatement implements DALStatement, MySQLStatement {
private String schema;
}
```
3. `DropIndexStatementAssert` `AlterIndexStatementAssert` and `CreateIndexStatementAssert`
```java
private static void assertIndex(final SQLCaseAssertContext assertContext, final DropIndexStatement actual, final DropIndexStatementTestCase expected) {
// TODO should assert index for all databases(mysql and sqlserver do not parse index right now)
}
```
Here we also need add logic in parser.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev commented on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev commented on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-750300900
@jingshanglu Hi, I have complete `expr` assert for `ExpectedJoinTable`.
And I found that some other segments also contain `expr`, but no test cases
Do these need to add test cases?
- [ ] assignmentValue
- [ ] call
- [ ] doStatement
- [ ] projection
```g4
assignmentValue
: expr | DEFAULT | blobValue
;
call
: CALL identifier (LP_ (expr (COMMA_ expr)*)? RP_)?
;
doStatement
: DO expr (COMMA_ expr)*
;
projection
: expr (AS? alias)? | qualifiedShorthand
;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] jingshanglu commented on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-734194083
@lwtdev Now, we can do it step by step, `Expr` is more important, we can fix assert of `Expr`,
- **Questions About** `ExpectedWhereClause`
1. Now , i think `ExpectedAndPredicate` and `ExpectedPredicate` in `ExpectedWhereClause` is unreasonable, maybe `ExpectedExpr` is better, and add some subclass for it, like `ExpectedInExpression`, `ExpectedNotExpression` and so on.
Because of the redesign of the `Expr` domain model, we need to design reasonably UT for `Expr`, How do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] terrymanu closed issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
terrymanu closed issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev edited a comment on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev edited a comment on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-750300900
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] jingshanglu commented on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-738644208
@lwtdev I agree with the design.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] wenweibin commented on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
wenweibin commented on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-723587484
hi @jingshanglu，in [PR#8043](https://github.com/apache/shardingsphere/pull/8043)，I just finished `MySQLCallStatement` refinement，so I want to know if other optimizations（for example `MySQLDoStatement,` other `DDL MySQL Statement`） need to continue？And if so, I'm willing to try to finish them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev edited a comment on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev edited a comment on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-733033027
@jingshanglu Hi, I'm trying to add TableAssert (for UpdateStatementAssert) OwnerAssit and WhereAssert in next pull request.
- **Questions About** `ExpectedWhereClause`
1. What's the different between `ExpectedAndPredicate` and `ExpectedPredicate` in `ExpectedWhereClause`?
2. Why does defined a `List` of `ExpectedAndPredicate` in `ExpectedPredicate` , In which situation do we need more then one `ExpectedAndPredicate` in `ExpectedWhereClause`?
3. What does ` ExpectedPredicate` correspond to, is it `predicate`, or `booleanPrimary` , or `expr` ?
4. Why does `ExpectedPredicate` only had `column-left-value` for left part, what about `expr ` like `1 = t_order.status`?
- **Other TODO Tags**
I also found some `TODO` tags that need not only add `*Assert` but should need to add parse logic. These changes have a lot of impact, so if we should fix them, I hope to fix it in other pull requests.
1. `AssignmentAssert`
```java
public static void assertIs(final SQLCaseAssertContext assertContext, final AssignmentSegment actual, final ExpectedAssignment expected) {
ColumnAssert.assertIs(assertContext, actual.getColumn(), expected.getColumn());
// TODO assert assign operator
AssignmentValueAssert.assertIs(assertContext, actual.getValue(), expected.getAssignmentValue());
}
```
Here need to add `operator` in `AssignmentSegment`, add change all test case that not contains `operator` before.
```g4
assignment
: columnName EQ_ assignmentValue
;
```
```java
public final class AssignmentSegment implements SQLSegment {
private final int startIndex;
private final int stopIndex;
private final ColumnSegment column;
private final ExpressionSegment value;
}
```
2 . `MySQLUseStatementAssert`
```java
public static void assertIs(final SQLCaseAssertContext assertContext, final MySQLUseStatement actual, final UseStatementTestCase expected) {
assertThat(assertContext.getText("Schema name assertion error: "), actual.getSchema(), is(expected.getSchema().getName()));
// TODO create a new assert class named `SchemaAssert`
// TODO extract and assert start index, stop index, start delimiter and end delimiter
}
```
Here need to change `String schema` to `SchemaSegment schema`. and add `visitSchemaSegmentContext` logic in visitor.
All code that references it also needs to be changed and tested.
```java
public final class MySQLUseStatement extends AbstractSQLStatement implements DALStatement, MySQLStatement {
private String schema;
}
```
3. `DropIndexStatementAssert` `AlterIndexStatementAssert` and `CreateIndexStatementAssert`
```java
private static void assertIndex(final SQLCaseAssertContext assertContext, final DropIndexStatement actual, final DropIndexStatementTestCase expected) {
// TODO should assert index for all databases(mysql and sqlserver do not parse index right now)
}
```
Here we also need add logic in parser.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] jingshanglu closed issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
jingshanglu closed issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev commented on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev commented on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-748853304
@jingshanglu Next step, I will replace all `<and-predicate>` to `<expr>`, and remove classes abount `*ExpectedPredicate*`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] jingshanglu commented on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-723715434
> hi @jingshanglu，in [PR#8043](https://github.com/apache/shardingsphere/pull/8043)，I just finished `MySQLCallStatement` refinement，so I want to know if other optimizations（for example `MySQLDoStatement,` other `DDL MySQL Statement`） need to continue？And if so, I'm willing to try to finish them.
@wenweibin Yes, we also need to optimize other statement definitions. The commonly used ones are `alterTable`, `createTable`. We can start with these statements. I will create some subtasks according to the order of usage frequency. In #7791. Welcome to join the shardingsphere community
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev edited a comment on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev edited a comment on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-729342237
> > > @lwtdev Now, there is no UT for expr, are you interested in fix it? The relevant issue is #7880
> >
> >
> > @jingshanglu I don't known what should to do about the work `UT for expr`, Which ‘expr’ need to add test cases?
>
> @lwtdev There is `WhereSegment` in selectStatement like this:
>
> ```
> public abstract class SelectStatement extends AbstractSQLStatement implements DMLStatement {
>
> private ProjectionsSegment projections;
>
> private TableSegment from;
>
> private WhereSegment where;
>
> private GroupBySegment groupBy;
>
> private OrderBySegment orderBy;
> }
> // WhereSegment contain expr
> public final class WhereSegment implements SQLSegment {
>
> private final int startIndex;
>
> private final int stopIndex;
>
> private final ExpressionSegment expr;
> }
> ```
>
> But in our test framework, there is no assertion of expr, like this:
>
> ```
> public final class WhereClauseAssert {
>
> /**
> * Assert actual where segment is correct with expected where clause.
> *
> * @param assertContext assert context
> * @param actual actual where segment
> * @param expected expected where clause
> */
> public static void assertIs(final SQLCaseAssertContext assertContext, final WhereSegment actual, final ExpectedWhereClause expected) {
> // TODO support expr in where assert
> }
> }
> ```
>
> just like `assertLockClause()` that you added.
@jingshanglu OK, I think I get it.
I will find `TODO` tag in the `shardingsphere-sql-parser-test` module. Then fix them?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] lwtdev edited a comment on issue #7880: Add UT for table and expr.
Posted by GitBox <gi...@apache.org>.
lwtdev edited a comment on issue #7880:
URL: https://github.com/apache/shardingsphere/issues/7880#issuecomment-733033027
@jingshanglu Hi, I'm trying to add TableAssert (for UpdateStatementAssert) OwnerAssit and WhereAssert in next pull request.
- **Questions About** `ExpectedWhereClause`
1. What's the different between `ExpectedAndPredicate` and `ExpectedPredicate` in `ExpectedWhereClause`?
2. Why does defined a `List` of `ExpectedAndPredicate` in `ExpectedWhereClause` , In which situation do we need more then one `ExpectedAndPredicate` in `ExpectedWhereClause`?
3. What does ` ExpectedPredicate` correspond to, is it `predicate`, or `booleanPrimary` , or `expr` ?
4. Why does `ExpectedPredicate` only had `column-left-value` for left part, what about `expr ` like `1 = t_order.status`?
- **Other TODO Tags**
I also found some `TODO` tags that need not only add `*Assert` but should need to add parse logic. These changes have a lot of impact, so if we should fix them, I hope to fix it in other pull requests.
1. `AssignmentAssert`
```java
public static void assertIs(final SQLCaseAssertContext assertContext, final AssignmentSegment actual, final ExpectedAssignment expected) {
ColumnAssert.assertIs(assertContext, actual.getColumn(), expected.getColumn());
// TODO assert assign operator
AssignmentValueAssert.assertIs(assertContext, actual.getValue(), expected.getAssignmentValue());
}
```
Here need to add `operator` in `AssignmentSegment`, add change all test case that not contains `operator` before.
```g4
assignment
: columnName EQ_ assignmentValue
;
```
```java
public final class AssignmentSegment implements SQLSegment {
private final int startIndex;
private final int stopIndex;
private final ColumnSegment column;
private final ExpressionSegment value;
}
```
2 . `MySQLUseStatementAssert`
```java
public static void assertIs(final SQLCaseAssertContext assertContext, final MySQLUseStatement actual, final UseStatementTestCase expected) {
assertThat(assertContext.getText("Schema name assertion error: "), actual.getSchema(), is(expected.getSchema().getName()));
// TODO create a new assert class named `SchemaAssert`
// TODO extract and assert start index, stop index, start delimiter and end delimiter
}
```
Here need to change `String schema` to `SchemaSegment schema`. and add `visitSchemaSegmentContext` logic in visitor.
All code that references it also needs to be changed and tested.
```java
public final class MySQLUseStatement extends AbstractSQLStatement implements DALStatement, MySQLStatement {
private String schema;
}
```
3. `DropIndexStatementAssert` `AlterIndexStatementAssert` and `CreateIndexStatementAssert`
```java
private static void assertIndex(final SQLCaseAssertContext assertContext, final DropIndexStatement actual, final DropIndexStatementTestCase expected) {
// TODO should assert index for all databases(mysql and sqlserver do not parse index right now)
}
```
Here we also need add logic in parser.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org