zendframework / zend-db

Db component from Zend Framework
BSD 3-Clause "New" or "Revised" License
101 stars 122 forks source link

Short named parameters #305

Closed hamilok closed 6 years ago

hamilok commented 6 years ago

Fixed bug "ORA-00972: identifier is too long"

hamilok commented 6 years ago

Before:

SELECT
  COUNT(1) AS "C"
FROM (
  SELECT
    "SYS_USER"."ID" AS "ID",
    "SYS_USER"."EMAIL" AS "EMAIL",
    "SYS_USER"."STATUS" AS "STATUS",
    TO_CHAR(LAST_SIGNIN_TIME, :subselect1subselect1LAST_SIGNIN_TIME1) AS "LAST_SIGNIN_TIME",
    TO_CHAR(CREATED_TIME, :subselect1subselect1CREATED_TIME1) AS "CREATED_TIME"
  FROM
    "SYS_USER"
  WHERE
    "LAST_SIGNIN_TIME" > SYSDATE - NUMTODSINTERVAL(:subselect1subselect1where1subpart1, :subselect1subselect1where1subpart2)
    AND
    "CREATED_TIME" > SYSDATE - NUMTODSINTERVAL(:subselect1subselect1where1subpart3, :subselect1subselect1where1subpart4
  )) "original_select"

After:

SELECT
  COUNT(1) AS "C"
FROM (
  SELECT
    "SYS_USER"."ID" AS "ID",
    "SYS_USER"."EMAIL" AS "EMAIL",
    "SYS_USER"."STATUS" AS "STATUS",
    TO_CHAR(LAST_SIGNIN_TIME, :ss1ss1LAST_SIGNIN_TIME1) AS "LAST_SIGNIN_TIME",
    TO_CHAR(CREATED_TIME, :ss1ss1CREATED_TIME1) AS "CREATED_TIME"
  FROM
    "SYS_USER"
  WHERE
    "LAST_SIGNIN_TIME" > SYSDATE - NUMTODSINTERVAL(:ss1ss1where1sp1, :ss1ss1where1sp2)
    AND
    "CREATED_TIME" > SYSDATE - NUMTODSINTERVAL(:ss1ss1where1sp3, :ss1ss1where1sp4
  )) "original_select"
michalbundyra commented 6 years ago

@hamilok Please check travis build: https://travis-ci.org/zendframework/zend-db/builds/342271399?utm_source=github_status&utm_medium=notification

It's failing because of your change. Could you please fix them and add your test case?

hamilok commented 6 years ago

@Ocramius please, merge it

Ocramius commented 6 years ago

This seems to patch just the prefix/suffix to shorten it, but adding multiple sub-selects will lead to the same issue anyway...

hamilok commented 6 years ago

Very likely, but now even with one "subselect" there is a problem, and this patch corrects it. @Ocramius What do you think?

ezimuel commented 6 years ago

@Ocramius are you sure that this doesn't fix also multiple sub-selects use cases?

Ocramius commented 6 years ago

@ezimuel yes: this is basically "let's fix an overflow by making the payload size smaller", instead of fixing the actual overflow.

ezimuel commented 6 years ago

@hamilok you mentioned bug ORA-00972, can you add more information on this? I don't see any issue opened with this reference. Thanks!

ezimuel commented 6 years ago

@Ocramius I see, anyway it's still a fix to reduce the issue. A better solution will be to rewrite the string parameters using some internal table reference, as I did here for #304.

Ocramius commented 6 years ago

anyway it's still a fix to reduce the issue

I'm extremely vary of anything that doesn't fix the issue at the source, so I can't agree with going with the current solution. The approach taken in #304 looks more appropriate.

ezimuel commented 6 years ago

@Ocramius you right, I'm closing this in favor of alternative solution based on #304 approach. @hamilok can you have a look at this solution and propose a new PR? Thanks anyway for your contribution here!

hamilok commented 6 years ago

@ezimuel solution based on #304 approach don't fix ORA-00972 problem.

What was in my changes?

The first type of placeholder is based on the name of the column: : subselect1subselect1LAST_SIGNIN_TIME1

16 user characters and 21 system characters. This is a clear overflow. It is necessary to reduce to a minimum of 5 system characters. Other characters for the user. : s1s1LAST_SIGNIN_TIME1

The second placeholder is auto-generated: : subselect1subselect1where1subpart1

Minimize up to 12 characters -: s1s1where1s1 Or up to 8 characters -: s1s1w1s1