Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.80) (envelope-from ) id 1aTkuw-0006sp-B1 for pgadmin-hackers@arkaria.postgresql.org; Thu, 11 Feb 2016 06:39:54 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84) (envelope-from ) id 1aTkuv-0007yL-PK for pgadmin-hackers@arkaria.postgresql.org; Thu, 11 Feb 2016 06:39:53 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84) (envelope-from ) id 1aTkuf-0007jC-Ti for pgadmin-hackers@postgresql.org; Thu, 11 Feb 2016 06:39:38 +0000 Received: from mail-ob0-x229.google.com ([2607:f8b0:4003:c01::229]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84) (envelope-from ) id 1aTkuX-00039K-OT for pgadmin-hackers@postgresql.org; Thu, 11 Feb 2016 06:39:36 +0000 Received: by mail-ob0-x229.google.com with SMTP id ba1so61268539obb.3 for ; Wed, 10 Feb 2016 22:39:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=oCkDqGbentZ+aPRRFo29vzWyEkA+senWfnVt9aS4UVw=; b=f6DU1RQvTTqRBYw0HUG+iRTBzU5PVAa3j7MZR9ozxFiODmN2s7rZ1fJMm9g9bHVLhl KNshIdRoJof8YOMZW001mfU8kdGhPiNuNlDeU0OYweQrxQTPLeZEmGdj4NIXzE53aEQ5 Fk92maTsULWY4VC3BWuWSgcuNCVjcwjge5BA8d7AwCB9+hwSla7Ce5HUpS9iAKvWA11Q OwF2Xa389mLBhuOXEPeYQg4XVDDiw4hmkCQYo8fGIbcpCIS8EFvkXG61RjpGV4fmbC0B uHEXkOmpqL/6edzq+Fw5L5BYqGgTou9mDLZf03f6XqYbBYu1L0xbVfkXbQ1j/OOTD19N Sb5w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=oCkDqGbentZ+aPRRFo29vzWyEkA+senWfnVt9aS4UVw=; b=QjILSqYNNvSMFgC8wnCsck6mkMgeNuwkITu+bdhHd1f9GM5Sjt/CEkhV2rm+MxygIK DBaNj8FHIbSM52BA4WyWLPR14ONshxRkvB+rsHH7ZcxPcR4ofUybTJFOYvLJ1SiiekhV +jivC5ByDyjcULuLK15S2ltUWYhrW6tPFd5WLgs+h7jcyvR0eBqLH5Bz4sPycsK2CLZr GdTRwedVDt2g0V/lDARCi+G0c7kJdNiePTAgnMCBW0IA0iFFwZBodDrimqGLBtPrw2PR DcjujGmRjNitfAfbxR+7DknGqv91yVMtYwGvwUr557Ib1D/Xn2dB9UF2ewaAb5mcE26B 3rSg== X-Gm-Message-State: AG10YOQmwVZWmhx2D4vq+ncXkrQgu+pNmvdXv7VQT6Q4PN9LmdbdUPIfoHm87aZtPUxVIZXLBI4EsIR0Hb1KA7oq MIME-Version: 1.0 X-Received: by 10.182.86.98 with SMTP id o2mr27802487obz.27.1455172767977; Wed, 10 Feb 2016 22:39:27 -0800 (PST) Received: by 10.202.59.197 with HTTP; Wed, 10 Feb 2016 22:39:27 -0800 (PST) In-Reply-To: References: Date: Thu, 11 Feb 2016 12:09:27 +0530 Message-ID: Subject: Re: patch for cast module From: Akshay Joshi To: Sanket Mehta Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary=089e0149d020ec4237052b78cf28 X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org --089e0149d020ec4237052b78cf28 Content-Type: text/plain; charset=UTF-8 Hi Sanket Most of the review comments has been resolved but I found some issues with this patch - When select some system cast it is showing wrong query, for example when select "abstime->date" in pgAdmin3 it is showing "AS ASSIGNMENT" and with your code it is showing "AS IMPLICIT". - For some of the system casts SQL query showing "WITH FUNCTION date" instead of "WITH FUNCTION date(abstime)" source type is not appended in query with new code. - For casts "bit->"bit"" function and target type is not listed. - When we create a new cast like "character->bytea" without selecting function, it creates successfully but when we select the newly created cast it shows the function "bpcharsend" in functions property. It may come for other combinations too, please verify. - Please fixed warnings in python file by using pep8 tool. On Mon, Feb 8, 2016 at 3:45 PM, Sanket Mehta wrote: > Hi Akshay, > > PFA the revised patch. > All the comments are inline. > > > Regards, > Sanket Mehta > Sr Software engineer > Enterprisedb > > On Fri, Feb 5, 2016 at 12:43 PM, Akshay Joshi < > akshay.joshi@enterprisedb.com> wrote: > >> Hi Sanket >> >> Below are the review comments >> >> - As "Show System Object" is not implemented yet, we should show all >> the objects by default. >> >> Done > >> >> - As in pgAdmin3 when click on Casts (Collection) node it should show >> only Name, Owner and Comments. With current code it is showing all the >> properties. >> >> Done.. Owner field is ignore as it is not a part of cast properties. > >> >> - Properties Tab contains only one control "Comment" can that be a >> part of the Definition tab??? >> - For some data type like "Character", "Integer", it is throwing >> error that data type doesn't exist. >> >> resolved > >> >> - If node is leaf node then it should not show (+) expand symbol. >> >> Done > >> >> - Remove extra lines from create.sql and update.sql files as it shown >> in the SQL tab as well. >> >> Ignored as it was suggested by Ashesh. > >> >> - When select any system cast it is not showing function in the >> function control. >> >> Resolved. > >> >> - If comment is already exist and we remove the comments, sql query >> not generated in the SQL tab while it is generating in pgAdmin3. >> >> Done. > > >> *Question*: With current implementation in "pgAdmin3" to create "Cast" >> user will have to select source type and target type and then click on OK >> button. If source and target type is not physically compatible, server will >> throw an error. I am not sure, but instead of that can we implement it like >> when user select the source type from combo box, target type combo will >> only show types which are physically compatible? >> > After consulting with db server team, it is clear that they do not > maintain any mapping for compatible source and target types. in postgresql, > they pick selected source and target type and check them for compatibility. > So its not possible to filter out target type based on selected source type. > >> >> >> >> On Thu, Feb 4, 2016 at 6:31 PM, Sanket Mehta < >> sanket.mehta@enterprisedb.com> wrote: >> >>> Hi Akshay, >>> >>> PFA the latest patch for Cast module. >>> Please do review it and let me know if anything is missing. >>> >>> >>> Regards, >>> Sanket Mehta >>> Sr Software engineer >>> Enterprisedb >>> >>> On Wed, Jan 20, 2016 at 5:03 PM, Sanket Mehta < >>> sanket.mehta@enterprisedb.com> wrote: >>> >>>> Hi Neel. >>>> >>>> PFA the revised patch which has changed according to your comments. >>>> Please do review it and let me know in case anything is missing. >>>> >>>> >>>> >>>> Regards, >>>> Sanket Mehta >>>> Sr Software engineer >>>> Enterprisedb >>>> >>>> On Wed, Jan 20, 2016 at 10:20 AM, Neel Patel < >>>> neel.patel@enterprisedb.com> wrote: >>>> >>>>> Hi Sanket, >>>>> >>>>> Below are the review comments. >>>>> >>>>> - When we edit any existing cast node then it gives error "*Response >>>>> object has no attribute strip*". This error is coming because >>>>> generated SQL is >>>>> wrong. >>>>> - Unnecessary debug logs are coming on console. Please remove >>>>> unnecessary debug logs. >>>>> - In some of the sql file, 'qtIdent' and 'qtLiteral' is not used. >>>>> Please check all the SQL files. >>>>> - "Delete" cast functionality is not working. Error is getting >>>>> displayed saying *"syntax error at or near "castsource"*. >>>>> - "Delete cascade" functionality is not working - error is getting >>>>> displayed saying *"The requested URL not found".* >>>>> - Do the proper comments, in some of the function like "script_load" , >>>>> comments are wrong. >>>>> - Is "configs" really required in __init__.py file ? We have not seen >>>>> any usage for this. Please remove it if it is not required. >>>>> - Remove commented code from the source file. >>>>> >>>>> Please check all the generated SQL statements . Test the basic >>>>> functionality of "create", "Edit" and "Delete" node before sending patch >>>>> file. >>>>> >>>>> Do let us know for any comments/issues. >>>>> >>>>> Thanks, >>>>> Neel Patel >>>>> >>>>> On Tue, Jan 19, 2016 at 8:06 PM, Sanket Mehta < >>>>> sanket.mehta@enterprisedb.com> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> PFA updated patch for cast module as per check list provided by Neel. >>>>>> Please do review it and let me know in case of anything is missing. >>>>>> >>>>>> >>>>>> >>>>>> Regards, >>>>>> Sanket Mehta >>>>>> Sr Software engineer >>>>>> Enterprisedb >>>>>> >>>>>> On Mon, Jan 18, 2016 at 7:16 PM, Sanket Mehta < >>>>>> sanket.mehta@enterprisedb.com> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> PFA patch for cast module. >>>>>>> Please do review it and let me know in case of any issue. >>>>>>> >>>>>>> >>>>>>> Regards, >>>>>>> Sanket Mehta >>>>>>> Sr Software engineer >>>>>>> Enterprisedb >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org >>>>>> ) >>>>>> To make changes to your subscription: >>>>>> http://www.postgresql.org/mailpref/pgadmin-hackers >>>>>> >>>>>> >>>>> >>>> >>> >>> >>> -- >>> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) >>> To make changes to your subscription: >>> http://www.postgresql.org/mailpref/pgadmin-hackers >>> >>> >> >> >> -- >> *Akshay Joshi* >> *Principal Software Engineer * >> >> >> >> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >> > > -- *Akshay Joshi* *Principal Software Engineer * *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* --089e0149d020ec4237052b78cf28 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Sanket<= /span>=C2=A0

Most of the review comments has been resolv= ed but I found some issues with this patch=C2=A0
  • When sel= ect some system cast it is showing wrong query, for example when select &qu= ot;abstime->date&= quot; in pgAdmin3 it is showing "AS ASSIGNMENT" and with your cod= e it is showing "AS IMPLICIT".
  • For some of the system cas= ts SQL query showing= "WITH FUNCTION date" instead of "WITH FUNCTION date(abstime)" source type = is not appended in query with new code.
  • For casts "bit->&qu= ot;bit"" function and target type is not listed.
  • When we = create a new cast like "character->bytea" without selecting function, it creates s= uccessfully but when we select the newly created cast it shows the function= "bpcharsend&qu= ot; in functions property. It may come for other combinations too, please v= erify.=C2=A0
  • Please fixed warnings in python file by using pep8 too= l. =C2=A0 =C2=A0

On Mon, Feb 8, 2016 at 3:45 PM, Sanket Mehta <sanket.mehta@enterprisedb.co= m> wrote:
Hi Akshay,
PFA the revised patch.
All the comments are i= nline.


Regards,
Sanket Mehta
= Sr Software engineer
Enterprisedb

On Fri, Feb 5, 2016 = at 12:43 PM, Akshay Joshi <akshay.joshi@enterprisedb.com&g= t; wrote:
Hi Sanket=C2=A0

Below are the review comments
  • = As "Show System Object" is not implemented yet, we should show al= l the objects by default.
Don= e
  • = As in pgAdmin3 when click on Casts (Collection) node it should show only Na= me, Owner and Comments. With current code it is showing all the properties.=
Done.. Owner field is ignore= as it is not a part of cast properties.
  • Properties Tab contains only one co= ntrol "Comment" can that be a part of the Definition tab???
    • <= li>For some data type like "Character", "Integer", it i= s throwing error that data type doesn't exist.
resolved
=
  • If node is leaf node then it should not show = (+) expand symbol.
Done
<= /div>
  • Remove= extra lines from create.sql and update.sql files= as it shown in the SQL tab as well.
Ignored as it was suggested by Ashesh.
  • When select any = system cast it is not showing function in the function control.
Resolved.
  • If comment is already exist and= we remove the comments, sql query not generated in the = SQL tab while it is generating in pgAdmin3.
Done.
=C2=A0
Question: With current implementation in= "pgAdmin3" to create "Cast" user will have to select s= ource type and target type and then click on OK button. If source and targe= t type is not physically compatible, server will throw an error. I am not s= ure, but instead of that can we implement it like when user select the sour= ce type from combo box, target type combo will only show types which are ph= ysically compatible?
After consulti= ng with db server team, it is clear that they do not maintain any mapping f= or compatible source and target types. in postgresql, they pick selected so= urce and target type and check them for compatibility. So its not possible = to filter out target type based on selected source type.

<= div>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=C2=A0

On Thu, Feb 4, 2016 at= 6:31 PM, Sanket Mehta <sanket.mehta@enterprisedb.com><= /span> wrote:
Hi Akshay,
=
PFA the latest patch for Cast module.
Please do review i= t and let me know if anything is missing.


Regards,
Sank= et Mehta
Sr Software engineer
Enterprisedb
<= /div>

On Wed, Jan 20, 2016 at 5:0= 3 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:
Hi Neel.

= PFA the revised patch which has changed according to your comments.
Please do review it and let me know in case anything is missing.



Regards,
Sanket Mehta
Sr= Software engineer
Enterprisedb

On Wed, Jan 20, 2016 at 10:= 20 AM, Neel Patel <neel.patel@enterprisedb.com> wr= ote:
Hi Sanket,

Below are the review comments.

- When we edit a= ny existing cast node then it gives error "Response object has no a= ttribute strip". This error is coming because generated SQL is=C2= =A0
=C2=A0 wrong.
- Unnecessary debug logs are coming on console. P= lease remove unnecessary debug logs.
- In some of the sql file, &= #39;qtIdent' and 'qtLiteral' is not used. Please check all the = SQL files.
- "Delete" cast functionality is not w= orking. Error is getting displayed saying "syntax error at or near = "castsource".=C2=A0
-=C2=A0"Delete cascade&quo= t; functionality is not working=C2=A0- error is getting displayed saying "The requested URL not found".
- Do the proper com= ments, in some of the function like "script_load" , comments are = wrong.
- Is "configs" really required in __init__.py fi= le ? We have not seen any usage for this. Please remove it if it is not req= uired.
- Remove commented code from the source file.
Please check all the generated SQL statements . Test the basic= functionality of "create", "Edit" and "Delete&quo= t; node before sending patch file.

Do let us know = for any comments/issues.

Thanks,
Neel Pa= tel

On Tue, Jan 19, 2016 at 8:06 PM, Sanket Mehta <sanket.meht= a@enterprisedb.com> wrote:
<= div dir=3D"ltr">Hi,

PFA updated patch for cast module as= per check list provided by Neel.
Please do review it and let me = know in case of anything is missing.



Regards,
Sanket Mehta
Sr Software engineer
En= terprisedb

On Mon, Jan 18, 2016 at 7:1= 6 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:
Hi,

PFA p= atch for cast module.
Please do review it and let me know in case= of any issue.


Regards,
Sanket Mehta
Sr Software engineer
Enterprisedb



--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers=






--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers=




--
Akshay Joshi
Principal Softw= are Engineer=C2=A0
<= span style=3D"color:rgb(0,0,0);font-family:arial,sans-serif;font-size:13px;= border-collapse:collapse">
<= /b>
=
Phone: +91 20-3058-9517
Mobile: +91 976-788-8246<= /font>




--
Aksha= y Joshi


<= b>Phone: +91 20-3058-9517
Mobile: +91 976-788= -8246
--089e0149d020ec4237052b78cf28--