You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by ashwinrayaprolu1984 <gi...@git.apache.org> on 2015/11/08 21:32:35 UTC
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
GitHub user ashwinrayaprolu1984 opened a pull request:
https://github.com/apache/metamodel/pull/72
Updating FromItemParser to parse tableNames with spaces
I had a requirement where tablename contains spaces.. And FromItemParser is throwing exceptions.
I made [] as token delimiters where i have [schema name.tablename]..
Example where i had issue:
Query q = dataContext.parseQuery("SELECT * FROM [Aqu Fall 2015 Comp Dates.xlsx.Sheet2]”);
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ashwinrayaprolu1984/metamodel master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metamodel/pull/72.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 #72
----
commit b0166f081485cf3ea3bdfcda73b1ac7f17b6c4e3
Author: ashwin <as...@northalley.com>
Date: 2015-11-08T20:28:24Z
Updating FromItemParser to parse tableNames with spaces
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by ashwinrayaprolu1984 <gi...@git.apache.org>.
Github user ashwinrayaprolu1984 commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-156571141
Note sure why this checkin is failing.. Debugging that.. Need to go out will be back in 15 min and check my repository
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by ashwinrayaprolu1984 <gi...@git.apache.org>.
Github user ashwinrayaprolu1984 commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-155097833
Will update in 2 hours and commit..
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/72#discussion_r45185504
--- Diff: core/src/main/java/org/apache/metamodel/query/QueryConstants.java ---
@@ -0,0 +1,41 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.metamodel.query;
+
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * This class shall hold all constant configurable variables related to Query
+ * parsing
+ *
+ * @author ashwinrayaprolu
+ *
+ */
+public final class QueryConstants {
+ /**
+ * This field will hold start and end character for delimiter that
+ * can be used
+ */
+ public static final Map<Character, Character> delimiterMap = new HashMap<Character, Character>();
--- End diff --
I think having a Map internally as a means towards the goal is fine, let's just not expose it as publically accessible API.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by ashwinrayaprolu1984 <gi...@git.apache.org>.
Github user ashwinrayaprolu1984 commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-156570966
Please review code in https://github.com/ashwinrayaprolu1984/metamodel/commit/2a7aa9b7f4a9398ce0822d7eccb1f2d208affe8d
core/src/main/java/org/apache/metamodel/query/QueryConstants.java
core/src/main/java/org/apache/metamodel/query/parser/FromItemParser.java
core/src/test/java/org/apache/metamodel/query/parser/QueryParserTest.java
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by ashwinrayaprolu1984 <gi...@git.apache.org>.
Github user ashwinrayaprolu1984 commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-155152411
I'll leave this decision up to you.. As you guys are owners of this.. But yeah i agree with @kaspersorensen that we should embrace everything... Will proceed after your comments
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-155358929
Thanks for the feedback @albertostratio. I think we can then come to an agreement that both syntaxes (double quotes and square brackets) should be parseable by the MetaModel query parser.
@ashwinrayaprolu1984 - For now I think you want this feature just for for table references, but in the future I would love to see it also for other references (columns and maybe more). Maybe if you can separate the tokenizer logic here so that we can reuse it for other parsing purposes. I imagine a tokenizer that would be delimiter aware, so that
* ```"foo bar" baz``` becomes ```foo bar``` and ```baz```
* ```[foo bar] baz``` becomes ```foo bar``` and ```baz```
* ```[foo bar] [baz]``` becomes ```foo bar``` and ```baz```
* ```[foo bar] baz "hello world"``` becomes ```foo bar``` and ```baz``` and ```hello world```
(already there you have the input for a unittest of the parser ;-))
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-155097130
I certainly agree that people has a tendency of screwing it up... Which is no wonder, as MySQL uses backtick as the default identifier quote, T-SQL uses brackets and when creating a table or column the argument is not an identifier but a string literal, so a quote would be consideren an explicit character (meaning that depending on the database, you might end up with a table or column with the quotes)...
However, when we generate the SQL, there shouldn't be a danger of that, so I think keeping to the standard as closely as possible would be a good thing.
@kaspersorensen (and any others feeling like chiming in), what would your take be?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/72
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by ashwinrayaprolu1984 <gi...@git.apache.org>.
Github user ashwinrayaprolu1984 commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-155143846
In this case i would prefer we defining metacharacter list in core API class which parse all token form its delimiters and returns value .
MetamodelSqlParser
{
extractTableName(inputTableName){
return parsedTableName;
}
........
}
This way we can centralize all parsing and use regex to parse expressions rather than this string api
@Regadring Java Query API: I prefer using that when i code in java but as i work with multiple languages i prefer to expose all my services to all languages..And to me SQL or some query language is best way to expose any data format..
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by ashwinrayaprolu1984 <gi...@git.apache.org>.
Github user ashwinrayaprolu1984 commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-155094566
I was actually going that route but from my experience i found people always make mistakes in escaping double quotes and hence took this route.. I can change this.. Its an architecture and designer decision.. So let me know if you want me to change and commit it to my version of code
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-155016092
The code looks good, but wouldn't it be better to use the SQL standard double quotes than the T-SQL specific brackets? I do believe T-SQL also supports those by default.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by ashwinrayaprolu1984 <gi...@git.apache.org>.
Github user ashwinrayaprolu1984 commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-155359607
Sure will proceed with above logic..
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-156789508
I would suggest to remove the CSV directory related things from this PR since its unrelated. Keep that in a different working branch.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-155147158
Something like that (using regex) may be possible for sure, but that's more an implementation concern. For now the question that I am most concerned about is the syntax/API that we expose. The moment we have agreement on that then we can even start with an inferior implementation and consider everything from there on an improvement. But the moment we expose it we also have people using it and expect us to maintain it, so I guess the question is also on what we want to maintain and what we think would be good syntax.
Personally, I like both syntaxes (double quotes and square brackets) for different reasons (double quotes seems to be the most wide-spread "standard" while square brackets is a bit more concise because it has a distinct beginning and end). So I would consider supporting both, in the spirit of MetaModel to "embrace all" of the database landscape :)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-155140230
For me an extremely hard question to answer.
As a java developer I anyways personally prefer the programmatic Java Query API over the string parser.
From a user perspective (on a product that expose the string parsing to the end user) I tend to think naively that "the more the better" in the sense of - if my user uses double quotes or square brackets - ideally MetaModel would be able to parse both/all, satisfying all users and creating the least amount of friction. The naive part is of course that at the same time it should be consistent and avoid horror scenarios where the meaning of a character might become ambigiuous.
@albertostratio I think you are also exposing the query parser to your end users? Could you maybe share your thoughts on the user of quotes, brackets etc. for delimiting table names (and eventually other references too).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by Ashwin Rayaprolu <as...@gmail.com>.
I made all changes and committed code with test cases added
I’m really not sure what is the issue with derby test.. No changes from my end. All files have same timestamp which is my initial checkout timestamp
Patches directory was mistake.. I was creating a patch file to send email and i accidentally created it in home directory which i deleted now.
Please check updated code in
https://github.com/ashwinrayaprolu1984/metamodel/commit/ab25ae8594bb582b170d6d2c65a217c5f5c5e024 <https://github.com/ashwinrayaprolu1984/metamodel/commit/ab25ae8594bb582b170d6d2c65a217c5f5c5e024>
Regards
Ashwin
> On Nov 8, 2015, at 2:50 PM, Ashwin Rayaprolu <as...@gmail.com> wrote:
>
> Regarding formatting: i’m talking more on these lines..
>
> http://code.revelc.net/formatter-maven-plugin/examples.html#Setting_Source_Files <http://code.revelc.net/formatter-maven-plugin/examples.html#Setting_Source_Files>
>
> If you ok to chat on any IM let me know what do you use. I would connect you there.
>
> Regards
> Ashwin
>> On Nov 8, 2015, at 2:46 PM, Ashwin Rayaprolu <ashwin.rayaprolu@gmail.com <ma...@gmail.com>> wrote:
>>
>> Hi,
>>
>> Even i was not sure why that DerbyTest got changed.
>>
>>
>> I initially checked out from https://git-wip-us.apache.org/repos/asf/metamodel.git <https://git-wip-us.apache.org/repos/asf/metamodel.git> and then migrated to a different repository url cloned from (https://github.com/apache/metamodel.git <https://github.com/apache/metamodel.git>). Maybe that was reason i see derby test got changed..
>> Not sure about .patch file
>> I’ll add unit test and checkin to my version of version control by end of day( Currently not at home)
>> Regarding final: Im extremely sorry for that..I was trying to implement some other logic and removed that at that point.. I’ll change that..Its my bad.
>> I’ll change it to
>> // From token can be starting with [
>> final String tableNameToken;
>> final String aliasToken
>>
>> Will add all checks.. I just wanted to make sure my changes are welcome and then proceed on all checks hence didn’t think much on that angle.. I’m planning to add more Datasource context’s in coming week. Already started working on that..I really appreciate you reviewing my code..
>> Regarding formatting: If you have any code formatter configured for maven that would probably standardize and remove this issue totally. Apart from that if you have formatter files for eclipse i’ll import them.. Will go through all rules and fix them.
>>
>>
>> Regards
>> Ashwin
>>
>>> On Nov 8, 2015, at 2:20 PM, kaspersorensen <git@git.apache.org <ma...@git.apache.org>> wrote:
>>>
>>> Github user kaspersorensen commented on the pull request:
>>>
>>> https://github.com/apache/metamodel/pull/72#issuecomment-154881570 <https://github.com/apache/metamodel/pull/72#issuecomment-154881570>
>>>
>>> Hi there,
>>>
>>> Thank you very much for the contribution! I think this is a good idea, but the PR is a bit questionable in it's current shape, so I hope you would be interested in maybe taking these feedback points?
>>>
>>> A few strange things in this PR which I think are technicalies/mistakes, but please clarify:
>>> * The DerbyTest file already exists in the master branch. Yet on the diff it seems it's been added in full. I can't figure out why that is and if something changed? Did you copy it, change something in it or?
>>> * The .patch file also seems to be included in this Pull Request. That's also kinda strange.
>>>
>>> So just focusing on the change you've done to FromItemParser (this seems to be the real "suggestion"):
>>> * I would love to see a unittest that uses your new code. We have a lot of existing parsing tests in for instance the class QueryParserTest. You can easily add one or two more cases of parsing with the square brackets.
>>> * You've removed the final keyword from "tableNameToken" and "aliasToken" and instead initialized them to empty strings. This seems like a degradation in code quality to me.
>>> * There's no check on whether the end-bracket is found or not. I can imagine a lot of difficult exception scenarios being thrown there without giving the user proper feedback. We should safe-guard such code quite a lot IMO.
>>> * The same holds for validation of the alias that may or may not be there. And for the fact that an alias should not have spaces in it.
>>> * On the code formatting side I have a few remarks:
>>> * Mixed use of tabs and spaces in indentation.
>>> * No spaces around "else" token is not how we format code.
>>>
>>>
>>> ---
>>> 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 infrastructure@apache.org <ma...@apache.org> or file a JIRA ticket
>>> with INFRA.
>>> ---
>>
>
Re: [GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by Ashwin Rayaprolu <as...@gmail.com>.
Regarding formatting: i’m talking more on these lines..
http://code.revelc.net/formatter-maven-plugin/examples.html#Setting_Source_Files <http://code.revelc.net/formatter-maven-plugin/examples.html#Setting_Source_Files>
If you ok to chat on any IM let me know what do you use. I would connect you there.
Regards
Ashwin
> On Nov 8, 2015, at 2:46 PM, Ashwin Rayaprolu <as...@gmail.com> wrote:
>
> Hi,
>
> Even i was not sure why that DerbyTest got changed.
>
>
> I initially checked out from https://git-wip-us.apache.org/repos/asf/metamodel.git <https://git-wip-us.apache.org/repos/asf/metamodel.git> and then migrated to a different repository url cloned from (https://github.com/apache/metamodel.git <https://github.com/apache/metamodel.git>). Maybe that was reason i see derby test got changed..
> Not sure about .patch file
> I’ll add unit test and checkin to my version of version control by end of day( Currently not at home)
> Regarding final: Im extremely sorry for that..I was trying to implement some other logic and removed that at that point.. I’ll change that..Its my bad.
> I’ll change it to
> // From token can be starting with [
> final String tableNameToken;
> final String aliasToken
>
> Will add all checks.. I just wanted to make sure my changes are welcome and then proceed on all checks hence didn’t think much on that angle.. I’m planning to add more Datasource context’s in coming week. Already started working on that..I really appreciate you reviewing my code..
> Regarding formatting: If you have any code formatter configured for maven that would probably standardize and remove this issue totally. Apart from that if you have formatter files for eclipse i’ll import them.. Will go through all rules and fix them.
>
>
> Regards
> Ashwin
>
>> On Nov 8, 2015, at 2:20 PM, kaspersorensen <git@git.apache.org <ma...@git.apache.org>> wrote:
>>
>> Github user kaspersorensen commented on the pull request:
>>
>> https://github.com/apache/metamodel/pull/72#issuecomment-154881570 <https://github.com/apache/metamodel/pull/72#issuecomment-154881570>
>>
>> Hi there,
>>
>> Thank you very much for the contribution! I think this is a good idea, but the PR is a bit questionable in it's current shape, so I hope you would be interested in maybe taking these feedback points?
>>
>> A few strange things in this PR which I think are technicalies/mistakes, but please clarify:
>> * The DerbyTest file already exists in the master branch. Yet on the diff it seems it's been added in full. I can't figure out why that is and if something changed? Did you copy it, change something in it or?
>> * The .patch file also seems to be included in this Pull Request. That's also kinda strange.
>>
>> So just focusing on the change you've done to FromItemParser (this seems to be the real "suggestion"):
>> * I would love to see a unittest that uses your new code. We have a lot of existing parsing tests in for instance the class QueryParserTest. You can easily add one or two more cases of parsing with the square brackets.
>> * You've removed the final keyword from "tableNameToken" and "aliasToken" and instead initialized them to empty strings. This seems like a degradation in code quality to me.
>> * There's no check on whether the end-bracket is found or not. I can imagine a lot of difficult exception scenarios being thrown there without giving the user proper feedback. We should safe-guard such code quite a lot IMO.
>> * The same holds for validation of the alias that may or may not be there. And for the fact that an alias should not have spaces in it.
>> * On the code formatting side I have a few remarks:
>> * Mixed use of tabs and spaces in indentation.
>> * No spaces around "else" token is not how we format code.
>>
>>
>> ---
>> 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 infrastructure@apache.org <ma...@apache.org> or file a JIRA ticket
>> with INFRA.
>> ---
>
Re: [GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by Ashwin Rayaprolu <as...@gmail.com>.
Hi,
Even i was not sure why that DerbyTest got changed.
I initially checked out from https://git-wip-us.apache.org/repos/asf/metamodel.git <https://git-wip-us.apache.org/repos/asf/metamodel.git> and then migrated to a different repository url cloned from (https://github.com/apache/metamodel.git). Maybe that was reason i see derby test got changed..
Not sure about .patch file
I’ll add unit test and checkin to my version of version control by end of day( Currently not at home)
Regarding final: Im extremely sorry for that..I was trying to implement some other logic and removed that at that point.. I’ll change that..Its my bad.
I’ll change it to
// From token can be starting with [
final String tableNameToken;
final String aliasToken
Will add all checks.. I just wanted to make sure my changes are welcome and then proceed on all checks hence didn’t think much on that angle.. I’m planning to add more Datasource context’s in coming week. Already started working on that..I really appreciate you reviewing my code..
Regarding formatting: If you have any code formatter configured for maven that would probably standardize and remove this issue totally. Apart from that if you have formatter files for eclipse i’ll import them.. Will go through all rules and fix them.
Regards
Ashwin
> On Nov 8, 2015, at 2:20 PM, kaspersorensen <gi...@git.apache.org> wrote:
>
> Github user kaspersorensen commented on the pull request:
>
> https://github.com/apache/metamodel/pull/72#issuecomment-154881570
>
> Hi there,
>
> Thank you very much for the contribution! I think this is a good idea, but the PR is a bit questionable in it's current shape, so I hope you would be interested in maybe taking these feedback points?
>
> A few strange things in this PR which I think are technicalies/mistakes, but please clarify:
> * The DerbyTest file already exists in the master branch. Yet on the diff it seems it's been added in full. I can't figure out why that is and if something changed? Did you copy it, change something in it or?
> * The .patch file also seems to be included in this Pull Request. That's also kinda strange.
>
> So just focusing on the change you've done to FromItemParser (this seems to be the real "suggestion"):
> * I would love to see a unittest that uses your new code. We have a lot of existing parsing tests in for instance the class QueryParserTest. You can easily add one or two more cases of parsing with the square brackets.
> * You've removed the final keyword from "tableNameToken" and "aliasToken" and instead initialized them to empty strings. This seems like a degradation in code quality to me.
> * There's no check on whether the end-bracket is found or not. I can imagine a lot of difficult exception scenarios being thrown there without giving the user proper feedback. We should safe-guard such code quite a lot IMO.
> * The same holds for validation of the alias that may or may not be there. And for the fact that an alias should not have spaces in it.
> * On the code formatting side I have a few remarks:
> * Mixed use of tabs and spaces in indentation.
> * No spaces around "else" token is not how we format code.
>
>
> ---
> 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 infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-154881570
Hi there,
Thank you very much for the contribution! I think this is a good idea, but the PR is a bit questionable in it's current shape, so I hope you would be interested in maybe taking these feedback points?
A few strange things in this PR which I think are technicalies/mistakes, but please clarify:
* The DerbyTest file already exists in the master branch. Yet on the diff it seems it's been added in full. I can't figure out why that is and if something changed? Did you copy it, change something in it or?
* The .patch file also seems to be included in this Pull Request. That's also kinda strange.
So just focusing on the change you've done to FromItemParser (this seems to be the real "suggestion"):
* I would love to see a unittest that uses your new code. We have a lot of existing parsing tests in for instance the class QueryParserTest. You can easily add one or two more cases of parsing with the square brackets.
* You've removed the final keyword from "tableNameToken" and "aliasToken" and instead initialized them to empty strings. This seems like a degradation in code quality to me.
* There's no check on whether the end-bracket is found or not. I can imagine a lot of difficult exception scenarios being thrown there without giving the user proper feedback. We should safe-guard such code quite a lot IMO.
* The same holds for validation of the alias that may or may not be there. And for the fact that an alias should not have spaces in it.
* On the code formatting side I have a few remarks:
* Mixed use of tabs and spaces in indentation.
* No spaces around "else" token is not how we format code.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-155357162
If we're going to add this feature I'd also go for both syntaxes. IMO it won't be a good idea to force the user to use a specific one.
@kaspersorensen you're right, the users of our web app type-in their queries and under-the-hood we rely on the metamodel query parser to decide whether the query is valid.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/72#discussion_r44868818
--- Diff: core/src/main/java/org/apache/metamodel/query/QueryConstants.java ---
@@ -0,0 +1,41 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.metamodel.query;
+
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * This class shall hold all constant configurable variables related to Query
+ * parsing
+ *
+ * @author ashwinrayaprolu
+ *
+ */
+public final class QueryConstants {
+ /**
+ * This field will hold start and end character for delimiter that
+ * can be used
+ */
+ public static final Map<Character, Character> delimiterMap = new HashMap<Character, Character>();
--- End diff --
Some concerns regarding this map/this class:
The map datatype works for the purpose of parsing but isn't really very self-explaining when it comes to understanding that key=start-token and value=end-token. The map is fine, but it's not API worthy.
Maps are mutable, so anyone can here add and remove delimiters. Bug or feature I don't know, but seems unintended and not like the right way to support that capability if we wanted it.
Being a public class and a public field it becomes API. I don't think we necessarily want to maintain an API for query parsing. The parsing is an internal thing mostly and who knows if we one day switch to use e.g. Antlr, javacc or whatever parsing framework.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by ashwinrayaprolu1984 <gi...@git.apache.org>.
Github user ashwinrayaprolu1984 commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/72#discussion_r45000717
--- Diff: core/src/main/java/org/apache/metamodel/query/QueryConstants.java ---
@@ -0,0 +1,41 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.metamodel.query;
+
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * This class shall hold all constant configurable variables related to Query
+ * parsing
+ *
+ * @author ashwinrayaprolu
+ *
+ */
+public final class QueryConstants {
+ /**
+ * This field will hold start and end character for delimiter that
+ * can be used
+ */
+ public static final Map<Character, Character> delimiterMap = new HashMap<Character, Character>();
--- End diff --
OK i can remove that class. I thought externalizing those delimiters.. Let me know if you are ok with me keeping the same logic in FormItemParser.parseTableItem(String itemToken) method. Or do you have any better solution? FYI: Deleted CSVDirectory related code.. Working on it locally. will commit once all my changes are done tested
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] metamodel pull request: Updating FromItemParser to parse tableName...
Posted by ashwinrayaprolu1984 <gi...@git.apache.org>.
Github user ashwinrayaprolu1984 commented on the pull request:
https://github.com/apache/metamodel/pull/72#issuecomment-156789594
Sure.. It got checked in by mistake.. Will remove those...
> On Nov 15, 2015, at 12:07 AM, Kasper Sørensen <no...@github.com> wrote:
>
> I would suggest to remove the CSV directory related things from this PR since its unrelated. Keep that in a different working branch.
>
> —
> Reply to this email directly or view it on GitHub <https://github.com/apache/metamodel/pull/72#issuecomment-156789508>.
>
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---