[IOTDB-174] Fix querying timeseries interface cannot make a query by the specified path prefix #435

tsunghanjacktsai · 2019-09-27T15:18:35Z

Corresponding to JIRA issue: https://issues.apache.org/jira/browse/IOTDB-174

Fix the bug which the querying timeseries number interface cannot make a query by the specified path prefix.
Fix the documentation corresponding to the query operation (add the device in path).

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

tsunghanjacktsai · 2019-10-01T12:45:30Z

I have added the SQLException to remind users if they enter the wrong child node.

LeiRui

Apart from the code reviews below, there are other problems in the code:

IoTDBStatement line 280 is a bug. If you query count nodes root.demo, you will get a message "Msg: 3", which is not expected.
In org.apache.iotdb.jdbc.Constant, COUNT_NODE_TIMESERIES = "cnttsbg". I guess "bg" is a typo for "gp"-groupby. To fix it, I suggest you change it to "cntnodets" for consistency.
In TSServiceImpl line 323, "COUNT_TIMESERIES" takes the same branch as that of "ALL_COLUMNS". However, "COUNT_TIMESERIES" only needs a count number rather than concrete path strings, so is it a waste of communication?

LeiRui · 2019-10-09T11:38:25Z

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

-  Set<String> getAllDevices() {
-    return new HashSet<>(getNodesList(3));
+  Set<String> getAllDevices() throws SQLException {
+    return new HashSet<>(getNodesList("root",2));


device is not necessarily at level 2.
device is at the secod to last level.
For example, CREATE TIMESERIES root.demo.m2.m2.s0 WITH DATATYPE=INT32,ENCODING=RLE and the device should be root.demo.m2.m2.

@LeiRui Hi, thanks for your detailed explanation. Actually I have no idea now about how to implement this interface as it was not developed by me before. What I done is just making sure that the program does not report the error.

However, I would like to discuss with you guys about how to make changes of this function, could you please give me some advices? I think this should create another PR for this or reopen PR #421 to correct this issue.

Hi, terribly sorry for the late reply, and I’m responsible for this issue. Do you think it is necessary to add a new param String schemaPattern for method Set<String> getAllDevices() as well?

Or maybe I will fix this bug after your PR is merged so that it won't result in too many conflicts since your PR is currently not reporting any error.

LeiRui · 2019-10-09T11:41:19Z

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

+    String[] nodes = schemaPattern.split("\\.");
+    MNode node;
+    if ((node = getRoot()) != null) {
+      for (int i = 1; i < nodes.length; i++) {


The start of the schemaPattern is not checked to be root.
I find that count nodes xx.demo level=2 still returns the count result as normal, which is not right.

LeiRui

Besides, since this feature is not only valid in jdbc, I suggest you also add relative documents in the docs/Documentation/UserGuide/5-IoTDB SQL Documentation/1-IoTDB Query Statement.md.

LeiRui · 2019-10-10T02:07:03Z

One more suggestion: the header of this pr is a little bit general. Make it more specific would be better.

jt2594838

Do not forget to add tests to cover the situations mentioned by Lei Rui.

jixuan1989

Please modify the SQL Reference document to add COUNT clause.

tsunghanjacktsai · 2019-10-17T01:09:08Z

Hi, I have no idea now about how to implement the interface which could show all devices (which is another issue) as it was not developed by me before. What I done here is just making sure that the program does not report the error.

However, I would like to discuss with you guys about how to make changes of this function, could you please give me some advices? I think this should create another PR for this or reopen PR #421 to correct this issue.

tsunghanjacktsai · 2019-10-17T01:26:06Z

Please modify the SQL Reference document to add COUNT clause.

I have added the new schema statement in it. Please have a look.

qiaojialin · 2019-10-22T01:19:47Z

server/src/main/java/org/apache/iotdb/db/metadata/MManager.java

   * @return A List instance which stores all node at given level
   */
-  public List<String> getNodesList(int nodeLevel) {
+  public List<String> getNodesList(String schemaPattern, int nodeLevel) throws SQLException {


Suggested change

public List<String> getNodesList(String schemaPattern, int nodeLevel) throws SQLException {

public List<String> getNodesList(String prefixPath, int nodeLevel) throws SQLException {

qiaojialin · 2019-10-22T01:20:56Z

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

-  Set<String> getAllDevices() {
-    return new HashSet<>(getNodesList(3));
+  Set<String> getAllDevices() throws SQLException {
+    List<String> res = new ArrayList<>();


Suggested change

List<String> res = new ArrayList<>();

List<String> devices = new ArrayList<>();

And, why not define a set directly

qiaojialin · 2019-10-22T01:23:45Z

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

   */
-  Set<String> getAllDevices() {
-    return new HashSet<>(getNodesList(3));
+  Set<String> getAllDevices() throws SQLException {


Suggested change

Set<String> getAllDevices() throws SQLException {

Set<String> getAllDevices() {

qiaojialin · 2019-10-22T01:32:57Z

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

+    List<String> res = new ArrayList<>();
+    MNode node;
+    if ((node = getRoot()) != null) {
+      findDevices(node, "root", res);


Suggested change

findDevices(node, "root", res);

findDevices(node, SQLConstant.ROOT, res);

qiaojialin · 2019-10-22T01:35:33Z

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

+      res.add(path);
+      return;
+    }
+    if (node.hasChildren()) {


You already check the node is null or is a leaf. Here, the node should have a child. This if statement is not needed.

qiaojialin · 2019-10-22T01:39:15Z

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

+    if (node.hasChildren()) {
+      for (MNode child : node.getChildren().values()) {
+        if (child.isLeaf()) {
+          findDevices(child, path, res);


Suggested change

findDevices(child, path, res);

res.add(path);

qiaojialin · 2019-10-22T01:40:25Z

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

+    return new LinkedHashSet<>(res);
+  }
+
+  private void findDevices(MNode node, String path, List<String> res) {


This function is very confused, what's the relationship between node and path?

@qiaojialin It might not have to declare two separate params here, which it could use node.getName() to get the path name of the specified node. However, the function above (findStorageGroup) has also declared like this, so I'm not sure if I have to change this declaration.

Or are you confusing with the naming of params? Is there any suggestion about how to change it to resolve the confusion?

Here path != node.getName() , path is the full path (e.g., root.d1.s1) of the node while node.getName only return the node name (e.g., s1).

But now, after you change the res.add(path), these two parameters are consistent. No need to change.

BTW, if you find some existing codes which are confusing, do not hesitate to optimize them.

qiaojialin · 2019-10-22T01:43:43Z

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

   * @return a list contains all nodes at the given level
   */
-  List<String> getNodesList(int nodeLevel) {
+  List<String> getNodesList(String schemaPattern, int nodeLevel) throws SQLException {


Is this function only for the count? If so, why not return an int?

No, it is not only for the count. The function "COUNT NODE ... GROUP BY ..." has also been shared with this method, as it needs paths (path string) in this list so that it could return the node number within the specified path under the specified level and the path name to users.

jt2594838 · 2019-10-22T02:59:15Z

There are failing tests.

jt2594838 reviewed Sep 29, 2019

View reviewed changes

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java Outdated Show resolved Hide resolved

LeiRui reviewed Oct 9, 2019

View reviewed changes

LeiRui requested changes Oct 9, 2019

View reviewed changes

jt2594838 reviewed Oct 10, 2019

View reviewed changes

jixuan1989 requested changes Oct 10, 2019

View reviewed changes

tsunghanjacktsai changed the title ~~[IOTDB-174] Fix querying timeseries bug~~ [IOTDB-174] Fix querying timeseries interface cannot make a query by the specified path prefix Oct 11, 2019

qiaojialin reviewed Oct 22, 2019

View reviewed changes

jt2594838 approved these changes Oct 22, 2019

View reviewed changes

jt2594838 self-requested a review October 22, 2019 02:58

[IOTDB-174] Fix querying timeseries bug

db09660

qiaojialin approved these changes Oct 25, 2019

View reviewed changes

qiaojialin merged commit a92e0d9 into apache:master Oct 25, 2019

	public List<String> getNodesList(String schemaPattern, int nodeLevel) throws SQLException {
	public List<String> getNodesList(String prefixPath, int nodeLevel) throws SQLException {

	List<String> res = new ArrayList<>();
	List<String> devices = new ArrayList<>();

	Set<String> getAllDevices() throws SQLException {
	Set<String> getAllDevices() {

	findDevices(node, "root", res);
	findDevices(node, SQLConstant.ROOT, res);

[IOTDB-174] Fix querying timeseries interface cannot make a query by the specified path prefix #435

[IOTDB-174] Fix querying timeseries interface cannot make a query by the specified path prefix #435

Uh oh!

Conversation

tsunghanjacktsai commented Sep 27, 2019

Uh oh!

Uh oh!

tsunghanjacktsai commented Oct 1, 2019

Uh oh!

LeiRui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsunghanjacktsai Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeiRui left a comment

Choose a reason for hiding this comment

Uh oh!

LeiRui commented Oct 10, 2019

Uh oh!

jt2594838 left a comment

Choose a reason for hiding this comment

Uh oh!

jixuan1989 left a comment

Choose a reason for hiding this comment

Uh oh!

tsunghanjacktsai commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tsunghanjacktsai commented Oct 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiaojialin Oct 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsunghanjacktsai Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsunghanjacktsai Oct 17, 2019 •

edited

Loading

tsunghanjacktsai commented Oct 17, 2019 •

edited

Loading

qiaojialin Oct 22, 2019 •

edited

Loading

tsunghanjacktsai Oct 23, 2019 •

edited

Loading

tsunghanjacktsai Oct 23, 2019 •

edited

Loading