[GitHub] drill pull request #987: DRILL-5863: Sortable table incorrectly sorts fragme...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/drill/pull/987


---

[GitHub] drill pull request #987: DRILL-5863: Sortable table incorrectly sorts fragme...

[kkhatua]

Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/987#discussion_r148126237
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -76,12 +77,14 @@ public String getId() {
   public String getContent() {
 TableBuilder builder = new TableBuilder(OPERATOR_COLUMNS, 
OPERATOR_COLUMNS_TOOLTIP, true);
 
+Map attributeMap = new HashMap(); 
//Reusing for different fragments
 for (ImmutablePair, String> ip 
: opsAndHosts) {
   int minor = ip.getLeft().getRight();
   OperatorProfile op = ip.getLeft().getLeft();
 
+  attributeMap.put("data-order", String.valueOf(minor)); //Overwrite 
values from previous fragments
--- End diff --

You are correct that the values must be zero padded. However, the padding 
by default is only up till achieving a width of two digits. The sequencing 
didn't get messed up as long as the number of minor fragments for each major 
fragment was less than 100.
For greater than 100, the sequencing broke the way you described it.
So, until this fix, you'll get the minor fragment ordered as: 
01-01-XX . 01-10-XX, 01-100-XX, 
instead of 
01-01-XX . 01-10-XX, 01-11-XX,  01-10-XX, 01-11-XX,  01-100-XX, 
01-101-XX

What we are doing is injecting the  element with an attribute that 
carries the actual numeric value. The dataTable library will discover and sort 
the elements of a column by the value of this attribute instead of the value 
that the element encapsulates.


---

[GitHub] drill pull request #987: DRILL-5863: Sortable table incorrectly sorts fragme...

[kkhatua]

Github user kkhatua commented on a diff in the pull request:

https://github.com/apache/drill/pull/987#discussion_r148123715
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -76,12 +77,14 @@ public String getId() {
   public String getContent() {
 TableBuilder builder = new TableBuilder(OPERATOR_COLUMNS, 
OPERATOR_COLUMNS_TOOLTIP, true);
 
+Map attributeMap = new HashMap(); 
//Reusing for different fragments
 for (ImmutablePair, String> ip 
: opsAndHosts) {
   int minor = ip.getLeft().getRight();
   OperatorProfile op = ip.getLeft().getLeft();
 
+  attributeMap.put("data-order", String.valueOf(minor)); //Overwrite 
values from previous fragments
   String path = new 
OperatorPathBuilder().setMajor(major).setMinor(minor).setOperator(op).build();
-  builder.appendCell(path);
+  builder.appendCell(path, attributeMap);
--- End diff --

The appendCell() method has been overloaded with to allow for a map, from 
which any set of attributes can be injected. 

https://github.com/kkhatua/drill/blob/783ca992f3bbb52ae606a8ca10745d6dd2ed0d4f/exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/TableBuilder.java#L95
This allows for any future libraries that might need to inject their own 
set of attributes to be incorporated without the need for any significant 
modification to the TableBuilder.
So, each appendCell is provided its own unique map that is consumed during 
the construction of the `` element


---

[GitHub] drill pull request #987: DRILL-5863: Sortable table incorrectly sorts fragme...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/987#discussion_r144914422
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -76,12 +77,14 @@ public String getId() {
   public String getContent() {
 TableBuilder builder = new TableBuilder(OPERATOR_COLUMNS, 
OPERATOR_COLUMNS_TOOLTIP, true);
 
+Map attributeMap = new HashMap(); 
//Reusing for different fragments
 for (ImmutablePair, String> ip 
: opsAndHosts) {
   int minor = ip.getLeft().getRight();
   OperatorProfile op = ip.getLeft().getLeft();
 
+  attributeMap.put("data-order", String.valueOf(minor)); //Overwrite 
values from previous fragments
   String path = new 
OperatorPathBuilder().setMajor(major).setMinor(minor).setOperator(op).build();
-  builder.appendCell(path);
+  builder.appendCell(path, attributeMap);
--- End diff --

Does `appendCell` copy the map contents? If not, if it simply holds onto 
the map, then all cells hold the same map, which is probably not what you want.


---

[GitHub] drill pull request #987: DRILL-5863: Sortable table incorrectly sorts fragme...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/987#discussion_r144914493
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/FragmentWrapper.java
 ---
@@ -244,7 +248,8 @@ public String getContent() {
 biggestBatches = Math.max(biggestBatches, batches);
   }
 
-  builder.appendCell(new 
OperatorPathBuilder().setMajor(major).setMinor(minor).build());
+  attributeMap.put("data-order", 
String.valueOf(minor.getMinorFragmentId())); //Overwrite values from previous 
fragments
--- End diff --

See note below.


---

[GitHub] drill pull request #987: DRILL-5863: Sortable table incorrectly sorts fragme...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/987#discussion_r144914241
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/OperatorWrapper.java
 ---
@@ -76,12 +77,14 @@ public String getId() {
   public String getContent() {
 TableBuilder builder = new TableBuilder(OPERATOR_COLUMNS, 
OPERATOR_COLUMNS_TOOLTIP, true);
 
+Map attributeMap = new HashMap(); 
//Reusing for different fragments
 for (ImmutablePair, String> ip 
: opsAndHosts) {
   int minor = ip.getLeft().getRight();
   OperatorProfile op = ip.getLeft().getLeft();
 
+  attributeMap.put("data-order", String.valueOf(minor)); //Overwrite 
values from previous fragments
--- End diff --

Not clear on how this works. If we are sorting on a string value, then the 
values must be zero padded so that 02 comes before 11. Otherwise, order will be 
1, 10, 11, 2, 3.


---

[GitHub] drill pull request #987: DRILL-5863: Sortable table incorrectly sorts fragme...

[kkhatua]

GitHub user kkhatua opened a pull request:

https://github.com/apache/drill/pull/987

DRILL-5863: Sortable table incorrectly sorts fragments/time lexically

The DataTables jQuery library sorts data based on the value of the element 
in a column.
However, since Drill publishes sortable items like fragment IDs and time 
durations as non-numeric text, the sorting is incorrect. 
This PR fixes the fragment and duration ordering based on their implicit 
numeric values (minor ID and millisecond representation, respectively).

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/kkhatua/drill DRILL-5863

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/987.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #987


commit dee20aae602f86d14ddb832e7185499556c6a1e1
Author: Kunal Khatua 
Date:   2017-10-12T00:17:03Z

DRILL-5863: Sortable table incorrectly sorts fragments/time lexically

The DataTables jQuery library sorts data based on the value of the element 
in a column.
However, since Drill publishes sortable items like fragment IDs and time 
durations as non-numeric text, the sorting is incorrect. 
This PR fixes the fragment and duration ordering based on their implicit 
numeric values (minor ID and millisecond representation, respectively).




---