[GitHub] drill pull request #987: DRILL-5863: Sortable table incorrectly sorts fragme...
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...
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...
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...
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...
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...
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...
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). ---