|
That sounds like a reasonable approach to me. It may turn out that the checks are in the parser right now. What you're proposing may have to happen at bind() time.
James F. Adams made changes - 13/May/07 05:19 PM
Attached is a patch that addresses the ability to create columns where the data type of the column is currently for internal use or otherwise not exposed to the user.
I added an isInternalTypeId method to TypeId and defined the following types as being for internal use: BOOLEAN NATIONAL CHAR NATIONAL LONGVARCHAR NATIONAL VARCHAR REF TINYINT USERDEFINED NCLOB CreateTableNode was updated to use this method to check the data type of each column. A new message was added. A new test was added. This patch does not address the situation with the VALUES statement desribed by Army. I am wondering if the VALUES statement should even be allowed to create rows with invalid decimal precision.
James F. Adams made changes - 13/May/07 05:35 PM
James F. Adams made changes - 13/May/07 05:36 PM
Hi James, your patch adds NATIONAL <CHAR> types also where as these are not supported by derby (see
Is their any specific reason for that? Hi James,
Thank you for your willingness to pick this up! I think it'd be great to fix this issue before the 10.3 release, to minimize impact on potential users. That said, I took a quick look at this issue on Friday and managed a small, incomplete patch that tried to solve this problem in a slightly different way. I never got around to commenting or posting because other things came up. I'm posting the *incomplete* patch that I threw together here. Note that right now the code just prints out an error "OOPS" instead of throwing an exception, and it doesn't have any test cases. It also does not catch all of the types that your patch catches--though it should be possible to extend it in that way. In any event, I thought I'd post it as an alternate approach ("food for thought") in case it's useful. The notable thing about d2605_inc_v1.patch is that it addresses the DECIMAL error that I mentioned on derby-dev. It also avoids the need to add a new variable to TypeId...although that's probably not a big deal. > I am wondering if the VALUES statement should even be allowed to create rows > with invalid decimal precision. I was wondering that, too. And I'm not sure what the answer is. But for what it's worth, it looks we can actually catch this particular scenario at bind time *if* the assumption is that the only way to create such a value is via a literal. That seems like a valid assumption to me, but I haven't investigated fully and thus I could be wrong. Feel free to use or ignore the incomplete code as you see fit :) Either way, thanks again for choosing to work on this issue.
A B made changes - 14/May/07 03:45 PM
James F. Adams made changes - 15/May/07 02:07 AM
A B,
I think I prefer your approach. I am updating my patch. I have however found a couple of interesting things with the precision/scale/maximum width check. 1) Given table foo (x double precision), the check fails for: create table bar(y) as select 2.0 * x from foo with no data because dtd.getScale() returns 1 and typeId.getMaximumScale() returns 0. The datat ype of column y is double precision. 2) The check fails for: create table bar as select javaclassname from sys.sysaliases with no data because dtd.getMaximumWidth() returns 2147483647 and typeId.getMaximumMaximumWidth() returns 32700. The data type of javaclassname is LONG VARCHAR. I would expect the maximum length to be 32700. Integer.MAX_VALUE is specified as the maximum length of this column in SYSALIASESRowFactory.java. I do not know if this is a bug or was intentional but the resulting maximum length certainly exceeds the documented value for a LONG VARCHAR. I am doing some more investigation. I should be able to upload an updated patch in a couple of days. > I have however found a couple of interesting things with the precision/scale/maximum width check.
Thank you for pointing these out! I forgot to mention that I didn't do any extensive testing of the incomplete patch that I posted. > 1) Given table foo (x double precision), the check fails for: <snip query> Good catch. I think that in this case there is an implicit scale/precision "check" because of the multiplication operation. For example, the following currently works (even though precision is > 31): ij> values 12345678901234567890123456789012345678901234; but if we make that part of another Derby operation--say multiplication--it fails: ij> values 12345678901234567890123456789012345678901234 * 1.0; ERROR 22003: The resulting value is outside the range for the data type DECIMAL/NUMERIC(31,1). The failure occurs as part of normal Derby processing for arithmetic operation. So I wonder if we only have to worry about cases where we have a standalone constant? If that's true then we could add a check for ConstantNode to the logic in CreateTableNode. Something like: DataTypeDescriptor dtd = (rc.getExpression() instanceof ConstantNode) ? rc.getExpressionType() : null; if ((dtd != null) && !dtd.isUserCreatableType()) <error> I haven't tested that approach, but I wonder if it might solve the 1st problem that you discovered. > 2) The check fails for: create table bar as select javaclassname from sys.sysaliases with no data > because dtd.getMaximumWidth() returns 2147483647. [...] The data type of javaclassname is > LONG VARCHAR. I would expect the maximum length to be 32700. Hmm, I would expect a max length of 32700, as well. > I do not know if this is a bug or was intentional but the resulting maximum length certainly > exceeds the documented value for a LONG VARCHAR. Sounds like a bug to me, though I do not know the history of this system table. There are two other system tables that have LONG VARCHAR columns: SYSTRIGGERS and SYSSTATEMENTS. The latter (SYSSTATEMENTS) uses TypeId.LONGVARCHAR_MAXWIDTH as the max width for the column, which I think is correct (that value is 32700). SYSTRIGGERS uses Integer.MAX_VALUE just like SYSALIASES, but I do not think that's correct. > I am doing some more investigation. I should be able to upload an updated patch in a couple of days. I appreciate your time with this. If you'd like to post an incremental patch which just addresses the type problem (ex. BOOLEAN) for now, I think that would be fine. You could then look into the scale/precision problem as a follow-up patch, if that makes things easier. Or you can just post a single patch--whichever you prefer. Attached is a new version of a patch for consideration.
James F. Adams made changes - 16/May/07 02:40 AM
James F. Adams made changes - 16/May/07 02:42 AM
Thank you for the updated patch, James. It looks good to me. I ran derbyall and suites.All on SUSE Linux with no new failures, so I went ahead and committed d2605_v2.patch with svn commit # 539164:
URL: http://svn.apache.org/viewvc?view=rev&rev=539164 I can't tell from the comments if there is another patch coming or not, so I'm not marking this issue as "resolved" yet. If you are done, please feel free to resolve it on your own... Thanks again for addressing this issue!
A B made changes - 17/May/07 11:25 PM
James F. Adams made changes - 18/May/07 10:31 AM
Daniel John Debrunner made changes - 27/Jun/07 02:23 PM
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
http://article.gmane.org/gmane.comp.apache.db.derby.devel/40881
I wonder if that issue and this one are related? Ex. Should the CREATE TABLE AS functionality be performing checks on the column types that it creates?