db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dag H. Wanvik (JIRA)" <j...@apache.org>
Subject [jira] Issue Comment Edited: (DERBY-481) implement SQL generated columns
Date Thu, 27 Nov 2008 02:52:46 GMT

    [ https://issues.apache.org/jira/browse/DERBY-481?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12651231#action_12651231
] 

dagw edited comment on DERBY-481 at 11/26/08 6:52 PM:
---------------------------------------------------------------

Reviewed derby-481-14-ab-dropColumn. Thanks again Rick!  The patch is
good to commit. My notes:

* AlterTableConstantAction.java:

- The following fragment threw me off at first:

> int sizeAfterCascadedDrops = td.getColumnDescriptorList().size() - cascadedDrops;
> // can NOT drop a column if it is the only one in the table
> if (sizeAfterCascadedDrops == 1)

Surely,, since we are talkign about size after cascaded drops, th test
should be against 0? But no, the quantity sizeAfterCascadedDrops does
not include the original dropped column, so the code works, but is a
bit misleading. Maybe do:

 int sizeAfterCascadedDrops = td.getColumnDescriptorList().size() - 1 - cascadedDrops;
 // Can not drop a column if it there would be no columns left:
 if (sizeAfterCascadedDrops == 0)

- Maybe it would be informative to add a comment on the recursive call
  that it never exceeds 2 in depth since generated columns don't
  reference other generated columns.

- The new error message LANG_CASCADED_GEN_COL_DROP doesn't appear to be
  used in the patch?

  Is the intention that it be used in this exception?

   throw StandardException.newException(
   SQLState.LANG_PROVIDER_HAS_DEPENDENT_OBJECT,
      dm.getActionString(DependencyManager.DROP_COLUMN),
      "THE *LAST* COLUMN " + columnName,
      "TABLE",
      td.getQualifiedName() );

  The new tests use LANG_PROVIDER_HAS_DEPENDENT_OBJECT (X0Y25), so
  perhaps LANG_CASCADED_GEN_COL_DROP is cruft?
  The wording of X0Y25 doesn't really precisely describe the problem,
  though, so a new error message would be nice, I think...

- The string "THE *LAST* COLUMN " is not i18n safe.

- If generated column were to be dropped indirectly, columnName could
  not *appear* to the user to be THE *LAST* COLUMN; I think the error
  should point out in such cases that, after cascade action, we would
  have 0 columns left. Perhaps the wording can be word-smithed to
  handle both cases well.

- The error message wording of LANG_CASCADED_GEN_COL_DROP is also
  confusing to me:

  "Dropping column '{0}' would orphan the generated column '{1}' which
  references it. You must drop the generated column first.

  I interpret this to mean that if 0 is dropped, 1 would be the only
  column left and a computed column can obviously not be alone in a
  table. But the issue would be clearer explained by saying that after
  1 is removed by cascade, there would be *no* columns left. I also
  don't understand the point about dropping the generated column
  first? Dropping 0 after having dropped 1 wouldn't improve things
  unless another column were added in the meantime? Or maybe I am
  misunderstanding here.. getting late ;-)

* GeneratedColumnsTest.java
 
   
- The verification that an index on a generated column is actually
  dropped could be better, I think. Presently, you only verify that
  an insert previosly blocked by the index is legal after the dropping
  of the column.  It would be good to inspect the dictionary to see
  that it is actually gone physically as well.

- Under verification that constraints are dropped, I started
  wondering.. I think this expected error is not according to the
  standard:

  :
  create table t_dc_10( a int generated always as ( -b ) check ( a is not null ), b int, c
int )"
  alter table t_dc_10 drop column a restrict  // gives X0Y25
  :

  According to section 11.18, we have:
  
  "5) If RESTRICT is specified, then C shall not be referenced in any of the following:
  a) The <query expression> of any view descriptor.

  b) The <search condition> of any constraint descriptor other than a
     table constraint descriptor that contains references to no other column and 
                                                          *********************************************************************
     that is included in the table descriptor of T.

  c) The SQL routine body of any routine descriptor.

  d) Either an explicit trigger column list or a triggered action
     column set of any trigger descriptor.

  e) The generation expression of any column descriptor."

  It would seem to be that in this case we are covered under item 5b),
  the check constraint only references the dropped column "a," so
  RESTRICT should not block this dropping. This is not
  behavior particular to generated columns, so if you agree with my interpretation, we should
  file a JIRA for it.

      was (Author: dagw):
    Reviewed derby-481-14-ab-dropColumn. Thanks again Rick!  The patch is
good to commit. My notes:

* AlterTableConstantAction.java:

- The following fragment threw me off at first:

> int sizeAfterCascadedDrops = td.getColumnDescriptorList().size() - cascadedDrops;
> // can NOT drop a column if it is the only one in the table
> if (sizeAfterCascadedDrops == 1)

Surely,, since we are talkign about size after cascaded drops, th test
should be against 0? But no, the quantity sizeAfterCascadedDrops does
not include the original dropped column, so the code works, but is a
bit misleading. Maybe do:

 int sizeAfterCascadedDrops = td.getColumnDescriptorList().size() - 1 - cascadedDrops;
 // Can not drop a column if it there would be no columns left:
 if (sizeAfterCascadedDrops == 0)

- Maybe it would be informative to add a comment on the recursive call
  that it never exceeds 2 in depth since generated columns don't
  reference other generated columns.

- The new error message LANG_CASCADED_GEN_COL_DROP doesn't appear to be
  used in the patch?

  Is the intention that it be used in this exception?

   throw StandardException.newException(
   SQLState.LANG_PROVIDER_HAS_DEPENDENT_OBJECT,
      dm.getActionString(DependencyManager.DROP_COLUMN),
      "THE *LAST* COLUMN " + columnName,
      "TABLE",
      td.getQualifiedName() );

  The new tests use LANG_PROVIDER_HAS_DEPENDENT_OBJECT (X0Y25), so
  perhaps LANG_CASCADED_GEN_COL_DROP is cruft?
  The wording of X0Y25 doesn't really precisely describe the problem,
  though, so a new error message would be nice, I think...

- The string "THE *LAST* COLUMN " is not i18n safe.

- If generated column were to be dropped indirectly, columnName could
  not *appear* to the user to be THE *LAST* COLUMN; I think the error
  should point out in such cases that, after cascade action, we would
  have 0 columns left. Perhaps the wording can be word-smithed to
  handle both cases well.

- The error message wording of LANG_CASCADED_GEN_COL_DROP is also
  confusing to me:

  "Dropping column '{0}' would orphan the generated column '{1}' which
  references it. You must drop the generated column first.

  I interpret this to mean that if 0 is dropped, 1 would be the only
  column left and a computed column can obviously not be alone in a
  table. But the issue would be clearer explained by saying that after
  1 is removed by cascade, there would be *no* columns left. I also
  don't understand the point about dropping the generated column
  first? Dropping 0 after having dropped 1 wouldn't improve things
  unless another column were added in the meantime? Or maybe I am
  misunderstanding here.. getting late ;-)

* GeneratedColumnsTest.java
 
   
- The verification that an index on a generated column is actually
  dropped could be better, I think. Presently, you only verify that
  an insert previosly blocked by the index is legal after the dropping
  of the column.  It would be good to inspect the dictionary to see
  that it is actually gone physically as well.

- Under verification that constraints are dropped, I started
  wondering.. I think this expected error is not according to the
  standard:

  :
  create table t_dc_10( a int generated always as ( -b ) check ( a is not null ), b int, c
int )"
  alter table t_dc_10 drop column a restrict  // gives X0Y25
  :

  According to section 11.19, we have:
  
  "5) If RESTRICT is specified, then C shall not be referenced in any of the following:
  a) The <query expression> of any view descriptor.

  b) The <search condition> of any constraint descriptor other than a
     table constraint descriptor that contains references to no other column and 
                                                          ****************** 
     that is included in the table descriptor of T.

  c) The SQL routine body of any routine descriptor.

  d) Either an explicit trigger column list or a triggered action
     column set of any trigger descriptor.

  e) The generation expression of any column descriptor."

  It would seem to be that in this case we are covered under item 5b),
  the check constraint only references the dropped column "a," so
  RESTRICT should not block this dropping. This is not
  behavior particular to generated columns, so if you agree with my interpretation, we should
  file a JIRA for it.
  
> implement SQL generated columns
> -------------------------------
>
>                 Key: DERBY-481
>                 URL: https://issues.apache.org/jira/browse/DERBY-481
>             Project: Derby
>          Issue Type: New Feature
>          Components: SQL
>    Affects Versions: 10.0.2.1
>            Reporter: Rick Hillegas
>            Assignee: Rick Hillegas
>         Attachments: derby-481-00-aa-prototype.diff, derby-481-01-aa-catalog.diff, derby-481-02-aa-utilities.diff,
derby-481-03-aa-grammar.diff, derby-481-04-aa-insert.diff, derby-481-05-aa-update.diff, derby-481-06-aa-genreferences.diff,
derby-481-07-aa-noSQLinRoutines.diff, derby-481-07-ab-noSQLinRoutines.diff, derby-481-08-aa-castToDeclaredType.diff,
derby-481-09-aa-dummyDefaults.diff, derby-481-10-aa-foreignKeyActions.diff, derby-481-11-aa-notNull.diff,
derby-481-12-aa-padding.diff, derby-481-13-aa-alterDatatype.diff, derby-481-14-ab-dropColumn.diff,
derby-481-15-aa-renameAndAddDefault.diff, derby-481-16-aa-dropFunction.diff, derby-481-17-aa-currentSchema.diff,
derby-481-18-aa-updatableResultSets.diff, derby-481-19-aa-cleanup.diff, GeneratedColumns.html,
GeneratedColumns.html, GeneratedColumns.html
>
>
> Satheesh has pointed out that generated columns, a SQL 2003 feature, would satisfy the
performance requirements of Expression Indexes (bug 455). Generated columns may not be as
elegant as Expression Indexes, but they are easier to implement. We would allow the following
new kind of column definition in CREATE TABLE and ALTER TABLE statements:
>     columnName GENERATED ALWAYS AS ( expression )
> If expression were an indexableExpression (as defined in bug 455), then we could create
indexes on it. There is no work for the optimizer to do here. The Language merely has to compute
the generated column at INSERT/UPDATE time.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message