[GitHub] drill pull request #775: DRILL-5326: Unit tests failures related to the SERV...
Github user zfong commented on a diff in the pull request: https://github.com/apache/drill/pull/775#discussion_r104805550 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/metadata/ServerMetaProvider.java --- @@ -76,7 +76,7 @@ .setReadOnly(false) .setGroupBySupport(GroupBySupport.GB_UNRELATED) .setLikeEscapeClauseSupported(true) - .setNullCollation(NullCollation.NC_AT_END) + .setNullCollation(NullCollation.NC_HIGH) --- End diff -- @jinfengni - See @vdiravka's comments in the Jira. The Drill documentation at https://drill.apache.org/docs/order-by-clause/#usage-notes says NULLs sort highest. If the doc is wrong, then we should fix the doc. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #748: DRILL-5263: Prevent left NLJoin with non scalar sub...
Github user zfong commented on a diff in the pull request: https://github.com/apache/drill/pull/748#discussion_r101637807 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/NestedLoopJoinPrule.java --- @@ -49,7 +49,7 @@ protected boolean checkPreconditions(DrillJoinRel join, RelNode left, RelNode ri PlannerSettings settings) { JoinRelType type = join.getJoinType(); -if (! (type == JoinRelType.INNER || type == JoinRelType.LEFT)) { +if (!(type == JoinRelType.INNER || (type == JoinRelType.LEFT && JoinUtils.hasScalarSubqueryInput(left, right { --- End diff -- @amansinha100 - @Serhii-Harnyk and I discussed this. If we return an error in the case I noted, then we will actually fail a bunch of existing unit tests. That's because there is Calcite code that flattens a subquery into an equivalent left outer join that uses scalar subqueries. Those queries end up failing. Given that, we will end up breaking queries that previously were able to run. Your thoughts on this? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #748: DRILL-5263: Prevent left NLJoin with non scalar sub...
Github user zfong commented on a diff in the pull request: https://github.com/apache/drill/pull/748#discussion_r101089177 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/NestedLoopJoinPrule.java --- @@ -49,7 +49,7 @@ protected boolean checkPreconditions(DrillJoinRel join, RelNode left, RelNode ri PlannerSettings settings) { JoinRelType type = join.getJoinType(); -if (! (type == JoinRelType.INNER || type == JoinRelType.LEFT)) { +if (!(type == JoinRelType.INNER || (type == JoinRelType.LEFT && JoinUtils.hasScalarSubqueryInput(left, right { --- End diff -- I see. So, Calcite is converting a query with a subquery into an equivalent left join with a scalar subquery. Do you know if the query returns the correct result in that case if a nested loop join is used? If not, then it seems like we should still be returning an error. Also, how do you distinguish the case where the query has been converted to this form by Calcite vs a user explicitly doing a left outer join to a scalar subquery. It seems like in this case, if Drill uses a nested loop join, it will also return wrong result. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #748: DRILL-5263: Prevent left NLJoin with non scalar sub...
Github user zfong commented on a diff in the pull request: https://github.com/apache/drill/pull/748#discussion_r101072686 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/NestedLoopJoinPrule.java --- @@ -49,7 +49,7 @@ protected boolean checkPreconditions(DrillJoinRel join, RelNode left, RelNode ri PlannerSettings settings) { JoinRelType type = join.getJoinType(); -if (! (type == JoinRelType.INNER || type == JoinRelType.LEFT)) { +if (!(type == JoinRelType.INNER || (type == JoinRelType.LEFT && JoinUtils.hasScalarSubqueryInput(left, right { --- End diff -- Don't we want to disallow left outer joins in all cases, even if one of the join inputs is a scalar? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #743: DRILL-5243: Fix TestContextFunctions.sessionIdUDFWithinSam...
Github user zfong commented on the issue: https://github.com/apache/drill/pull/743 Looks good to me. +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #666: DRILL-4956: Temporary tables support
Github user zfong commented on the issue: https://github.com/apache/drill/pull/666 Checked with @paul-rogers, he's ok with the changes. I'll add the ready-to-commit tag to the Jira. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #689: DRILL-4938: Need better error message
Github user zfong commented on the issue: https://github.com/apache/drill/pull/689 Can you add a testcase for this? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #675: DRILL-5094: Comparator should guarantee transitive attribu...
Github user zfong commented on the issue: https://github.com/apache/drill/pull/675 Looks good to me. +1 (non-binding) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #647: DRILL-4935 Allow Drill to advertise a specific hostname to...
Github user zfong commented on the issue: https://github.com/apache/drill/pull/647 @harrisonmebane - for pull requests submitted by non-committers, each week, one of the committers will do a batch commit of changes that have gone through review. This is normally done towards the end of the week. There may be an exception this week because as @paul-rogers has noted, we're in the middle of trying to push out a release candidate build for the 1.9 release. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #518: DRILL-4653.json - Malformed JSON should not stop the entir...
Github user zfong commented on the issue: https://github.com/apache/drill/pull/518 @paul-rogers - since you had concerns about particular test cases, and it looks like you've confirmed that those are non-issues, would it make sense for you to share those with @ssriniva123 and he can then include them as unit tests with this pull request? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #598: DRILL-4906 CASE Expression with constant generates class e...
Github user zfong commented on the issue: https://github.com/apache/drill/pull/598 @amansinha100 - can you review this pull request. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: fix obdc => odbc spelling
Github user zfong commented on the pull request: https://github.com/apache/drill/pull/505#issuecomment-220633886 @bbevens -- can you review. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4577: Construct a specific path for quer...
Github user zfong commented on the pull request: https://github.com/apache/drill/pull/461#issuecomment-210493669 Yes, 8 seconds may still be too long, but it's certainly better than what was there before. We've also been working with Simba to add an "includeSchema" option to the ODBC driver so tools that query from INFORMATION_SCHEMA can further restrict the query to only the relevant tables within the specified schema. That together with this patch should reduce the time significantly. Further improvements may still be needed even beyond this, but I suggest we address those incrementally rather than all at once. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4364: Image Metadata Format Plugin
Github user zfong commented on the pull request: https://github.com/apache/drill/pull/367#issuecomment-200871277 @vkorukanti, @jaltekruse, @adityakishore - since each of you have written format plugins in the past, can one of you help code review this change? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-4006: Ensure the position of MapWriters ...
Github user zfong commented on the pull request: https://github.com/apache/drill/pull/233#issuecomment-153421208 Is it possible to add a unit test for this problem? Seems like a pretty straightforward repro. Thanks. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3947: Use setSafe() for date, time, time...
Github user zfong commented on the pull request: https://github.com/apache/drill/pull/208#issuecomment-149259793 Aman -- yup, saw that. Thanks. On Mon, Oct 19, 2015 at 8:57 AM, Aman Sinha wrote: > @zfong <https://github.com/zfong>, please see my last explanation about > why the repro does not occur at small scale. Hence, adding a unit test > won't help ... unless the static constant that controls the initial size of > the value vectors is made configurable...but that would require more code > changes. > > â > Reply to this email directly or view it on GitHub > <https://github.com/apache/drill/pull/208#issuecomment-149257300>. > --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request: DRILL-3947: Use setSafe() for date, time, time...
Github user zfong commented on the pull request: https://github.com/apache/drill/pull/208#issuecomment-149087340 Is there a small unit test case that reproduces this problem? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---