Re: [Spacewalk-devel] Patch: add arch to channel.list*Channels
On 4/6/10 9:55 AM, Colin Coe wrote: > Hi all > > As per the API Addition page, I've added 'arch' to the > channel.list*Channels API calls. > > Comments/criticisms welcome > > CC > > > > > ___ > Spacewalk-devel mailing list > Spacewalk-devel@redhat.com > https://www.redhat.com/mailman/listinfo/spacewalk-devel Hey Colin, Having it do the lookups of package Arch in the Serializer really isn't the right way to do it as Serializers are just meant to translate the existing data. I would either add joins to rhnChannelArch to get the label in each of the queries or write a elaborator that just gets the channel arch and make each of those queries use the elaborator. Thanks, -Justin -- Justin Sherrill, RHCA 1801 Varsity Drive. Software EngineerRaleigh, NC 27603 Red Hat, Inc. ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] Patch: add arch to channel.list*Channels
Hi Justin Thanks for the feedback. I'll have another look at this and re-submit. CC On Thu, Apr 8, 2010 at 1:21 AM, Justin Sherrill wrote: > On 4/6/10 9:55 AM, Colin Coe wrote: >> Hi all >> >> As per the API Addition page, I've added 'arch' to the >> channel.list*Channels API calls. >> >> Comments/criticisms welcome >> >> CC >> >> >> >> >> ___ >> Spacewalk-devel mailing list >> Spacewalk-devel@redhat.com >> https://www.redhat.com/mailman/listinfo/spacewalk-devel > Hey Colin, > > Having it do the lookups of package Arch in the Serializer really isn't > the right way to do it as Serializers are just meant to translate the > existing data. > > I would either add joins to rhnChannelArch to get the label in each of > the queries or write a elaborator that just gets the channel arch and > make each of those queries use the elaborator. > > Thanks, > > -Justin > > > -- > Justin Sherrill, RHCA 1801 Varsity Drive. > Software Engineer Raleigh, NC 27603 > Red Hat, Inc. > > ___ > Spacewalk-devel mailing list > Spacewalk-devel@redhat.com > https://www.redhat.com/mailman/listinfo/spacewalk-devel > ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] Patch: add arch to channel.list*Channels
Hi Justin Is this a bit better? http://fpaste.org/LJwb/ Thanks CC On Thu, Apr 8, 2010 at 5:49 AM, Colin Coe wrote: > Hi Justin > > Thanks for the feedback. I'll have another look at this and re-submit. > > CC > > On Thu, Apr 8, 2010 at 1:21 AM, Justin Sherrill wrote: >> On 4/6/10 9:55 AM, Colin Coe wrote: >>> Hi all >>> >>> As per the API Addition page, I've added 'arch' to the >>> channel.list*Channels API calls. >>> >>> Comments/criticisms welcome >>> >>> CC >>> >>> >>> >>> >>> ___ >>> Spacewalk-devel mailing list >>> Spacewalk-devel@redhat.com >>> https://www.redhat.com/mailman/listinfo/spacewalk-devel >> Hey Colin, >> >> Having it do the lookups of package Arch in the Serializer really isn't >> the right way to do it as Serializers are just meant to translate the >> existing data. >> >> I would either add joins to rhnChannelArch to get the label in each of >> the queries or write a elaborator that just gets the channel arch and >> make each of those queries use the elaborator. >> >> Thanks, >> >> -Justin >> >> >> -- >> Justin Sherrill, RHCA 1801 Varsity Drive. >> Software Engineer Raleigh, NC 27603 >> Red Hat, Inc. >> >> ___ >> Spacewalk-devel mailing list >> Spacewalk-devel@redhat.com >> https://www.redhat.com/mailman/listinfo/spacewalk-devel >> > -- RHCE#805007969328369 ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] Patch: add arch to channel.list*Channels
OK, I think this patch is OK now. CC On Thu, Apr 8, 2010 at 9:10 PM, Colin Coe wrote: > Hi Justin > > Is this a bit better? http://fpaste.org/LJwb/ > > Thanks > > CC > > > > On Thu, Apr 8, 2010 at 5:49 AM, Colin Coe wrote: >> Hi Justin >> >> Thanks for the feedback. I'll have another look at this and re-submit. >> >> CC >> >> On Thu, Apr 8, 2010 at 1:21 AM, Justin Sherrill wrote: >>> On 4/6/10 9:55 AM, Colin Coe wrote: Hi all As per the API Addition page, I've added 'arch' to the channel.list*Channels API calls. Comments/criticisms welcome CC ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel >>> Hey Colin, >>> >>> Having it do the lookups of package Arch in the Serializer really isn't >>> the right way to do it as Serializers are just meant to translate the >>> existing data. >>> >>> I would either add joins to rhnChannelArch to get the label in each of >>> the queries or write a elaborator that just gets the channel arch and >>> make each of those queries use the elaborator. >>> >>> Thanks, >>> >>> -Justin >>> >>> >>> -- >>> Justin Sherrill, RHCA 1801 Varsity Drive. >>> Software Engineer Raleigh, NC 27603 >>> Red Hat, Inc. >>> >>> ___ >>> Spacewalk-devel mailing list >>> Spacewalk-devel@redhat.com >>> https://www.redhat.com/mailman/listinfo/spacewalk-devel >>> >> > > > > -- > RHCE#805007969328369 > -- RHCE#805007969328369 From cf421954e3eaa908d447bfe95fec1119fd4e5c03 Mon Sep 17 00:00:00 2001 From: Colin Coe Date: Tue, 6 Apr 2010 21:40:59 +0800 Subject: [PATCH] Add 'arch' to channel.list*Channels --- .../common/db/datasource/xml/Channel_queries.xml | 18 -- .../redhat/rhn/frontend/dto/ChannelTreeNode.java | 15 +++ .../serializer/ChannelTreeNodeSerializer.java |5 + 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/java/code/src/com/redhat/rhn/common/db/datasource/xml/Channel_queries.xml b/java/code/src/com/redhat/rhn/common/db/datasource/xml/Channel_queries.xml index b165f76..784fa57 100644 --- a/java/code/src/com/redhat/rhn/common/db/datasource/xml/Channel_queries.xml +++ b/java/code/src/com/redhat/rhn/common/db/datasource/xml/Channel_queries.xml @@ -410,7 +410,8 @@ SELECT CFO.id, CFO.org_id, CFO.name, CFO.label, CFO.current_members, CFO.max_mem C.org_id, (select org.name from web_customer org -where org.id = C.org_id) as org_name +where org.id = C.org_id) as org_name, + (select CA.name from rhnChannelArch CA where CA.ID = C.channel_arch_id) arch_name from rhnChannel C inner join rhnUserChannel UC on UC.channel_id = C.id where UC.user_id = :user_id @@ -431,7 +432,8 @@ SELECT CFO.id, CFO.org_id, CFO.name, CFO.label, CFO.current_members, CFO.max_mem C.org_id, (select org.name from web_customer org -where org.id = C.org_id) as org_name +where org.id = C.org_id) as org_name, + (select CA.name from rhnChannelArch CA where CA.ID = C.channel_arch_id) arch_name from rhnSharedChannelView C inner join rhnUserChannel UC on UC.channel_id = C.id where UC.user_id = :user_id @@ -452,7 +454,8 @@ SELECT CFO.id, CFO.org_id, CFO.name, CFO.label, CFO.current_members, CFO.max_mem C.org_id, (select org.name from web_customer org -where org.id = C.org_id) as org_name +where org.id = C.org_id) as org_name, + (select CA.name from rhnChannelArch CA where CA.ID = C.channel_arch_id) arch_name from rhnChannel C inner join rhnUserChannel UC on UC.channel_id = C.id where UC.user_id = :user_id AND @@ -479,7 +482,8 @@ SELECT CFO.id, CFO.org_id, CFO.name, CFO.label, CFO.current_members, CFO.max_mem C.org_id, (select org.name from web_customer org -where org.id = C.org_id) as org_name +where org.id = C.org_id) as org_name, + (select CA.name from rhnChannelArch CA where CA.ID = C.channel_arch_id) arch_name from rhnChannel C inner join rhnUserChannel UC on UC.channel_id = C.id where UC.user_id = :user_id and @@ -499,7 +503,8 @@ SELECT CFO.id, CFO.org_id, CFO.name, CFO.label, CFO.current_members, CFO.max_mem (SELECT COUNT(P.package_id) FROM rhnChannelPackage P WHERE P.channel_id = C.id - ) AS package_count + ) AS package_count, + (select CA.name from rhnChannelArch CA where CA.ID = C.channel_arch_id) arch_name from rhnChannel C inner join rhnUserChannel UC on UC.channel_id = C.id left join rhnChannel C2 on C.parent_channel = C2.id @@ -518,7 +523,8 @@ SELECT CFO.id, CFO.org_id, CFO.name, CFO.label, CFO.current_members, CFO.max_mem (S
Re: [Spacewalk-devel] Patch: add arch to channel.list*Channels
Hey Colin, why don't you just list all the data you need in the SELECT section and the tables that are necessary for the select in the FROM section? What I mean is to write: SELECT DISTINCT C.id, C.name, C.parent_channel AS parent_id, C.label AS channel_label, (SELECT COUNT(P.package_id) FROM rhnChannelPackage P WHERE P.channel_id = C.id ) AS package_count, CA.name AS arch_name FROM rhnChannel C inner join rhnChannelArch CA ON CA.ID = C.channel_arch_id inner join rhnUserChannel UC ON UC.channel_id = C.id left join rhnChannel C2 ON C2.parent_channel = C.id WHERE UC.user_id = :user_id AND (C.org_id = :org_id OR ( C2.id IS NOT NULL AND C2.org_id = :org_id )) instead of: select Distinct C.id, C.name, C.parent_channel as parent_id, C.label as channel_label, (SELECT COUNT(P.package_id) FROM rhnChannelPackage P WHERE P.channel_id = C.id ) AS package_count, (select CA.name from rhnChannelArch CA where CA.ID = C.channel_arch_id) arch_name from rhnChannel C inner join rhnUserChannel UC on UC.channel_id = C.id left join rhnChannel C2 on C2.parent_channel = C.id where UC.user_id = :user_id AND (C.org_id = :org_id or ( C2.id is not null AND C2.org_id = :org_id )) Regards, Tomas -- Tomas Lestach RHN Satellite Engineering, Red Hat - "Colin Coe" wrote: > OK, I think this patch is OK now. > > CC > > On Thu, Apr 8, 2010 at 9:10 PM, Colin Coe > wrote: > > Hi Justin > > > > Is this a bit better? http://fpaste.org/LJwb/ > > > > Thanks > > > > CC > > > > > > > > On Thu, Apr 8, 2010 at 5:49 AM, Colin Coe > wrote: > >> Hi Justin > >> > >> Thanks for the feedback. I'll have another look at this and > re-submit. > >> > >> CC > >> > >> On Thu, Apr 8, 2010 at 1:21 AM, Justin Sherrill > wrote: > >>> On 4/6/10 9:55 AM, Colin Coe wrote: > Hi all > > As per the API Addition page, I've added 'arch' to the > channel.list*Channels API calls. > > Comments/criticisms welcome > > CC > > > > > ___ > Spacewalk-devel mailing list > Spacewalk-devel@redhat.com > https://www.redhat.com/mailman/listinfo/spacewalk-devel > >>> Hey Colin, > >>> > >>> Having it do the lookups of package Arch in the Serializer really > isn't > >>> the right way to do it as Serializers are just meant to translate > the > >>> existing data. > >>> > >>> I would either add joins to rhnChannelArch to get the label in > each of > >>> the queries or write a elaborator that just gets the channel arch > and > >>> make each of those queries use the elaborator. > >>> > >>> Thanks, > >>> > >>> -Justin > >>> > >>> > >>> -- > >>> Justin Sherrill, RHCA 1801 Varsity Drive. > >>> Software Engineer Raleigh, NC 27603 > >>> Red Hat, Inc. > >>> > >>> ___ > >>> Spacewalk-devel mailing list > >>> Spacewalk-devel@redhat.com > >>> https://www.redhat.com/mailman/listinfo/spacewalk-devel > >>> > >> > > > > > > > > -- > > RHCE#805007969328369 > > > > > > -- > RHCE#805007969328369 > > ___ > Spacewalk-devel mailing list > Spacewalk-devel@redhat.com > https://www.redhat.com/mailman/listinfo/spacewalk-devel ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] Patch: add arch to channel.list*Channels
Umm, I guess because my SQL skills are on par with my Java skills... CC On Mon, Apr 12, 2010 at 4:21 PM, Tomas Lestach wrote: > Hey Colin, > > why don't you just list all the data you need in the SELECT section and the > tables that are necessary for the select in the FROM section? > > What I mean is to write: > > SELECT DISTINCT C.id, > C.name, > C.parent_channel AS parent_id, > C.label AS channel_label, > (SELECT COUNT(P.package_id) > FROM rhnChannelPackage P > WHERE P.channel_id = C.id > ) AS package_count, > CA.name AS arch_name > FROM rhnChannel C > inner join rhnChannelArch CA ON CA.ID = C.channel_arch_id > inner join rhnUserChannel UC ON UC.channel_id = C.id > left join rhnChannel C2 ON C2.parent_channel = C.id > WHERE UC.user_id = :user_id AND > (C.org_id = :org_id OR ( C2.id IS NOT NULL AND C2.org_id = :org_id > )) > > > instead of: > > select Distinct C.id, > C.name, > C.parent_channel as parent_id, > C.label as channel_label, > (SELECT COUNT(P.package_id) > FROM rhnChannelPackage P > WHERE P.channel_id = C.id > ) AS package_count, > (select CA.name from rhnChannelArch CA where CA.ID = > C.channel_arch_id) arch_name > from rhnChannel C inner join > rhnUserChannel UC on UC.channel_id = C.id left join > rhnChannel C2 on C2.parent_channel = C.id > where UC.user_id = :user_id AND > (C.org_id = :org_id or ( C2.id is not null AND C2.org_id = > :org_id )) > > > > Regards, > Tomas > > -- > Tomas Lestach > RHN Satellite Engineering, Red Hat > > - "Colin Coe" wrote: > >> OK, I think this patch is OK now. >> >> CC >> >> On Thu, Apr 8, 2010 at 9:10 PM, Colin Coe >> wrote: >> > Hi Justin >> > >> > Is this a bit better? http://fpaste.org/LJwb/ >> > >> > Thanks >> > >> > CC >> > >> > >> > >> > On Thu, Apr 8, 2010 at 5:49 AM, Colin Coe >> wrote: >> >> Hi Justin >> >> >> >> Thanks for the feedback. I'll have another look at this and >> re-submit. >> >> >> >> CC >> >> >> >> On Thu, Apr 8, 2010 at 1:21 AM, Justin Sherrill >> wrote: >> >>> On 4/6/10 9:55 AM, Colin Coe wrote: >> Hi all >> >> As per the API Addition page, I've added 'arch' to the >> channel.list*Channels API calls. >> >> Comments/criticisms welcome >> >> CC >> >> >> >> >> ___ >> Spacewalk-devel mailing list >> Spacewalk-devel@redhat.com >> https://www.redhat.com/mailman/listinfo/spacewalk-devel >> >>> Hey Colin, >> >>> >> >>> Having it do the lookups of package Arch in the Serializer really >> isn't >> >>> the right way to do it as Serializers are just meant to translate >> the >> >>> existing data. >> >>> >> >>> I would either add joins to rhnChannelArch to get the label in >> each of >> >>> the queries or write a elaborator that just gets the channel arch >> and >> >>> make each of those queries use the elaborator. >> >>> >> >>> Thanks, >> >>> >> >>> -Justin >> >>> >> >>> >> >>> -- >> >>> Justin Sherrill, RHCA 1801 Varsity Drive. >> >>> Software Engineer Raleigh, NC 27603 >> >>> Red Hat, Inc. >> >>> >> >>> ___ >> >>> Spacewalk-devel mailing list >> >>> Spacewalk-devel@redhat.com >> >>> https://www.redhat.com/mailman/listinfo/spacewalk-devel >> >>> >> >> >> > >> > >> > >> > -- >> > RHCE#805007969328369 >> > >> >> >> >> -- >> RHCE#805007969328369 >> >> ___ >> Spacewalk-devel mailing list >> Spacewalk-devel@redhat.com >> https://www.redhat.com/mailman/listinfo/spacewalk-devel > > ___ > Spacewalk-devel mailing list > Spacewalk-devel@redhat.com > https://www.redhat.com/mailman/listinfo/spacewalk-devel -- RHCE#805007969328369 ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] Patch: add arch to channel.list*Channels
Tomas, apologies for the original reply. My SQL skills are quite weak and doing the query wit joins did not occur to me. Is using the joins more efficient? I can't see a difference in the result returned when I run either of these queries. Thanks CC On Mon, Apr 12, 2010 at 4:21 PM, Tomas Lestach wrote: > Hey Colin, > > why don't you just list all the data you need in the SELECT section and the > tables that are necessary for the select in the FROM section? > > What I mean is to write: > > SELECT DISTINCT C.id, > C.name, > C.parent_channel AS parent_id, > C.label AS channel_label, > (SELECT COUNT(P.package_id) > FROM rhnChannelPackage P > WHERE P.channel_id = C.id > ) AS package_count, > CA.name AS arch_name > FROM rhnChannel C > inner join rhnChannelArch CA ON CA.ID = C.channel_arch_id > inner join rhnUserChannel UC ON UC.channel_id = C.id > left join rhnChannel C2 ON C2.parent_channel = C.id > WHERE UC.user_id = :user_id AND > (C.org_id = :org_id OR ( C2.id IS NOT NULL AND C2.org_id = :org_id > )) > > > instead of: > > select Distinct C.id, > C.name, > C.parent_channel as parent_id, > C.label as channel_label, > (SELECT COUNT(P.package_id) > FROM rhnChannelPackage P > WHERE P.channel_id = C.id > ) AS package_count, > (select CA.name from rhnChannelArch CA where CA.ID = > C.channel_arch_id) arch_name > from rhnChannel C inner join > rhnUserChannel UC on UC.channel_id = C.id left join > rhnChannel C2 on C2.parent_channel = C.id > where UC.user_id = :user_id AND > (C.org_id = :org_id or ( C2.id is not null AND C2.org_id = > :org_id )) > > > > Regards, > Tomas > > -- > Tomas Lestach > RHN Satellite Engineering, Red Hat > > - "Colin Coe" wrote: > >> OK, I think this patch is OK now. >> >> CC >> >> On Thu, Apr 8, 2010 at 9:10 PM, Colin Coe >> wrote: >> > Hi Justin >> > >> > Is this a bit better? http://fpaste.org/LJwb/ >> > >> > Thanks >> > >> > CC >> > >> > >> > >> > On Thu, Apr 8, 2010 at 5:49 AM, Colin Coe >> wrote: >> >> Hi Justin >> >> >> >> Thanks for the feedback. I'll have another look at this and >> re-submit. >> >> >> >> CC >> >> >> >> On Thu, Apr 8, 2010 at 1:21 AM, Justin Sherrill >> wrote: >> >>> On 4/6/10 9:55 AM, Colin Coe wrote: >> Hi all >> >> As per the API Addition page, I've added 'arch' to the >> channel.list*Channels API calls. >> >> Comments/criticisms welcome >> >> CC >> >> >> >> >> ___ >> Spacewalk-devel mailing list >> Spacewalk-devel@redhat.com >> https://www.redhat.com/mailman/listinfo/spacewalk-devel >> >>> Hey Colin, >> >>> >> >>> Having it do the lookups of package Arch in the Serializer really >> isn't >> >>> the right way to do it as Serializers are just meant to translate >> the >> >>> existing data. >> >>> >> >>> I would either add joins to rhnChannelArch to get the label in >> each of >> >>> the queries or write a elaborator that just gets the channel arch >> and >> >>> make each of those queries use the elaborator. >> >>> >> >>> Thanks, >> >>> >> >>> -Justin >> >>> >> >>> >> >>> -- >> >>> Justin Sherrill, RHCA 1801 Varsity Drive. >> >>> Software Engineer Raleigh, NC 27603 >> >>> Red Hat, Inc. >> >>> >> >>> ___ >> >>> Spacewalk-devel mailing list >> >>> Spacewalk-devel@redhat.com >> >>> https://www.redhat.com/mailman/listinfo/spacewalk-devel >> >>> >> >> >> > >> > >> > >> > -- >> > RHCE#805007969328369 >> > >> >> >> >> -- >> RHCE#805007969328369 >> >> ___ >> Spacewalk-devel mailing list >> Spacewalk-devel@redhat.com >> https://www.redhat.com/mailman/listinfo/spacewalk-devel > > ___ > Spacewalk-devel mailing list > Spacewalk-devel@redhat.com > https://www.redhat.com/mailman/listinfo/spacewalk-devel -- RHCE#805007969328369 ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] Patch: add arch to channel.list*Channels
On Tue, Apr 13, 2010 at 05:58:41AM +0800, Colin Coe wrote: > Tomas, apologies for the original reply. > > My SQL skills are quite weak and doing the query wit joins did not > occur to me. Is using the joins more efficient? I can't see a > difference in the result returned when I run either of these queries. Since there is a foreign key on rhnChannel(channel_arch_id) pointing to rhnChannelArch(id), the queries will return exactly the same results. There is nothing wrong with your syntax or your query. The only issue might be that when you or someone else will further tweak the query to for example return rhnChannelArch.label as well, with your query they might end up doing another inline select, instead of using the rhnChannelArch from the FROM list. And since the foreign key ensures that the cardinality of the result is not changed by joining with the rhnChannelArch table, it's actually a tiny bit better to do it the way Tomas has suggested. -- Jan Pazdziora Principal Software Engineer, Satellite Engineering, Red Hat ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] Patch: add arch to channel.list*Channels
- "Colin Coe" wrote: > Tomas, apologies for the original reply. Did you write something, you should apologize for? > My SQL skills are quite weak and doing the query wit joins did not > occur to me. Is using the joins more efficient? I can't see a > difference in the result returned when I run either of these queries. Colin, my SQL skills are similar to yours, so do not worry. That's why I consulted the patch with our DB guru, before I wrote you. :-) The outcome was: - result of the sql query is the same - even if the join version looks more gracefully, oracle shall optimize both variants the same, so it's not true one of the queries would run faster - BUT - the query looks much readable, if you first list the data you'd like to get and then the tables you want to get the data from, than to have nested selects - Jan's argument is part of that The main reason I wrote you wasn't I'd want to criticize you or your work. It was just a comment. Please, take it as a friendly reference. You can consider it or not. We know the spw code is not perfect and you can find many examples, where nested sql commands are used, where another constructions would fit better, but our intention is to constantly improve it. And a good start is to write code, that doesn't have to be corrected later on for any reason. So please, do not take our comments wrong. I just wanted to put near our coding conventions to you. Your work IS appreciated. Best Regards, Tomas > > Thanks > > CC > > On Mon, Apr 12, 2010 at 4:21 PM, Tomas Lestach > wrote: > > Hey Colin, > > > > why don't you just list all the data you need in the SELECT section > and the tables that are necessary for the select in the FROM section? > > > > What I mean is to write: > > > > SELECT DISTINCT C.id, > > C.name, > > C.parent_channel AS parent_id, > > C.label AS channel_label, > > (SELECT COUNT(P.package_id) > > FROM rhnChannelPackage P > > WHERE P.channel_id = C.id > > ) AS package_count, > > CA.name AS arch_name > > FROM rhnChannel C > > inner join rhnChannelArch CA ON CA.ID = C.channel_arch_id > > inner join rhnUserChannel UC ON UC.channel_id = C.id > > left join rhnChannel C2 ON C2.parent_channel = C.id > > WHERE UC.user_id = :user_id AND > > (C.org_id = :org_id OR ( C2.id IS NOT NULL AND C2.org_id > = :org_id )) > > > > > > instead of: > > > > select Distinct C.id, > > C.name, > > C.parent_channel as parent_id, > > C.label as channel_label, > > (SELECT COUNT(P.package_id) > > FROM rhnChannelPackage P > > WHERE P.channel_id = C.id > > ) AS package_count, > > (select CA.name from rhnChannelArch CA where CA.ID > = C.channel_arch_id) arch_name > > from rhnChannel C inner join > > rhnUserChannel UC on UC.channel_id = C.id left join > > rhnChannel C2 on C2.parent_channel = C.id > > where UC.user_id = :user_id AND > > (C.org_id = :org_id or ( C2.id is not null AND C2.org_id > = :org_id )) > > > > > > > > Regards, > > Tomas > > > > -- > > Tomas Lestach > > RHN Satellite Engineering, Red Hat > > > > - "Colin Coe" wrote: > > > >> OK, I think this patch is OK now. > >> > >> CC > >> > >> On Thu, Apr 8, 2010 at 9:10 PM, Colin Coe > >> wrote: > >> > Hi Justin > >> > > >> > Is this a bit better? http://fpaste.org/LJwb/ > >> > > >> > Thanks > >> > > >> > CC > >> > > >> > > >> > > >> > On Thu, Apr 8, 2010 at 5:49 AM, Colin Coe > >> wrote: > >> >> Hi Justin > >> >> > >> >> Thanks for the feedback. I'll have another look at this and > >> re-submit. > >> >> > >> >> CC > >> >> > >> >> On Thu, Apr 8, 2010 at 1:21 AM, Justin Sherrill > >> wrote: > >> >>> On 4/6/10 9:55 AM, Colin Coe wrote: > >> Hi all > >> > >> As per the API Addition page, I've added 'arch' to the > >> channel.list*Channels API calls. > >> > >> Comments/criticisms welcome > >> > >> CC > >> > >> > >> > >> > >> ___ > >> Spacewalk-devel mailing list > >> Spacewalk-devel@redhat.com > >> https://www.redhat.com/mailman/listinfo/spacewalk-devel > >> >>> Hey Colin, > >> >>> > >> >>> Having it do the lookups of package Arch in the Serializer > really > >> isn't > >> >>> the right way to do it as Serializers are just meant to > translate > >> the > >> >>> existing data. > >> >>> > >> >>> I would either add joins to rhnChannelArch to get the label in > >> each of > >> >>> the queries or write a elaborator that just gets the channel > arch > >> and > >> >>> make each of those queries use the elaborator. > >> >>> > >> >>> Thanks, > >> >>> > >> >>> -Justin > >> >>> > >> >>> > >> >>> -- > >> >>> Justin Sherrill, RHCA 1801 Varsity Drive. > >> >>> Software Engineer Raleigh, NC 27603 > >>
Re: [Spacewalk-devel] Patch: add arch to channel.list*Channels
Tomas and Jan, I'm grateful that I have the opportunity to contribute to software that I use and genuinely like. My apologies again because my response was unprofessional and unhelpful. As I'm learning that kind of feedback is what I need. Thanks for the explanation of the SQL, I still don't really get it yet but I'll review it again and google up on the joins. CC On Tue, Apr 13, 2010 at 4:40 PM, Tomas Lestach wrote: > - "Colin Coe" wrote: > >> Tomas, apologies for the original reply. > > Did you write something, you should apologize for? > >> My SQL skills are quite weak and doing the query wit joins did not >> occur to me. Is using the joins more efficient? I can't see a >> difference in the result returned when I run either of these queries. > > Colin, > my SQL skills are similar to yours, so do not worry. That's why I consulted > the patch with our DB guru, before I wrote you. :-) > The outcome was: > - result of the sql query is the same > - even if the join version looks more gracefully, oracle shall optimize both > variants the same, so it's not true one of the queries would run faster > - BUT - the query looks much readable, if you first list the data you'd like > to get and then the tables you want to get the data from, than to have nested > selects > - Jan's argument is part of that > > The main reason I wrote you wasn't I'd want to criticize you or your work. It > was just a comment. Please, take it as a friendly reference. You can consider > it or not. > > We know the spw code is not perfect and you can find many examples, where > nested sql commands are used, where another constructions would fit better, > but our intention is to constantly improve it. And a good start is to write > code, that doesn't have to be corrected later on for any reason. > > So please, do not take our comments wrong. I just wanted to put near our > coding conventions to you. > > Your work IS appreciated. > > > Best Regards, > Tomas > >> >> Thanks >> >> CC >> >> On Mon, Apr 12, 2010 at 4:21 PM, Tomas Lestach >> wrote: >> > Hey Colin, >> > >> > why don't you just list all the data you need in the SELECT section >> and the tables that are necessary for the select in the FROM section? >> > >> > What I mean is to write: >> > >> > SELECT DISTINCT C.id, >> > C.name, >> > C.parent_channel AS parent_id, >> > C.label AS channel_label, >> > (SELECT COUNT(P.package_id) >> > FROM rhnChannelPackage P >> > WHERE P.channel_id = C.id >> > ) AS package_count, >> > CA.name AS arch_name >> > FROM rhnChannel C >> > inner join rhnChannelArch CA ON CA.ID = C.channel_arch_id >> > inner join rhnUserChannel UC ON UC.channel_id = C.id >> > left join rhnChannel C2 ON C2.parent_channel = C.id >> > WHERE UC.user_id = :user_id AND >> > (C.org_id = :org_id OR ( C2.id IS NOT NULL AND C2.org_id >> = :org_id )) >> > >> > >> > instead of: >> > >> > select Distinct C.id, >> > C.name, >> > C.parent_channel as parent_id, >> > C.label as channel_label, >> > (SELECT COUNT(P.package_id) >> > FROM rhnChannelPackage P >> > WHERE P.channel_id = C.id >> > ) AS package_count, >> > (select CA.name from rhnChannelArch CA where CA.ID >> = C.channel_arch_id) arch_name >> > from rhnChannel C inner join >> > rhnUserChannel UC on UC.channel_id = C.id left join >> > rhnChannel C2 on C2.parent_channel = C.id >> > where UC.user_id = :user_id AND >> > (C.org_id = :org_id or ( C2.id is not null AND C2.org_id >> = :org_id )) >> > >> > >> > >> > Regards, >> > Tomas >> > >> > -- >> > Tomas Lestach >> > RHN Satellite Engineering, Red Hat >> > >> > - "Colin Coe" wrote: >> > >> >> OK, I think this patch is OK now. >> >> >> >> CC >> >> >> >> On Thu, Apr 8, 2010 at 9:10 PM, Colin Coe >> >> wrote: >> >> > Hi Justin >> >> > >> >> > Is this a bit better? http://fpaste.org/LJwb/ >> >> > >> >> > Thanks >> >> > >> >> > CC >> >> > >> >> > >> >> > >> >> > On Thu, Apr 8, 2010 at 5:49 AM, Colin Coe >> >> wrote: >> >> >> Hi Justin >> >> >> >> >> >> Thanks for the feedback. I'll have another look at this and >> >> re-submit. >> >> >> >> >> >> CC >> >> >> >> >> >> On Thu, Apr 8, 2010 at 1:21 AM, Justin Sherrill >> >> wrote: >> >> >>> On 4/6/10 9:55 AM, Colin Coe wrote: >> >> Hi all >> >> >> >> As per the API Addition page, I've added 'arch' to the >> >> channel.list*Channels API calls. >> >> >> >> Comments/criticisms welcome >> >> >> >> CC >> >> >> >> >> >> >> >> >> >> ___ >> >> Spacewalk-devel mailing list >> >> Spacewalk-devel@redhat.com >> >> https://www.redhat.com/mailman/listinfo/spacewalk-devel >> >> >>> Hey Colin, >> >> >>> >> >> >>> Having it do the lookups of packa
Re: [Spacewalk-devel] Patch: add arch to channel.list*Channels
Hi all I've just commited '9870a978d31f2f704385daa69c4177c89391b239' which changes the selects from nested to inner joins. CC On Mon, Apr 12, 2010 at 4:21 PM, Tomas Lestach wrote: > Hey Colin, > > why don't you just list all the data you need in the SELECT section and the > tables that are necessary for the select in the FROM section? > > What I mean is to write: > >SELECT DISTINCT C.id, > C.name, > C.parent_channel AS parent_id, > C.label AS channel_label, >(SELECT COUNT(P.package_id) > FROM rhnChannelPackage P > WHERE P.channel_id = C.id >) AS package_count, > CA.name AS arch_name >FROM rhnChannel C > inner join rhnChannelArch CA ON CA.ID = C.channel_arch_id > inner join rhnUserChannel UC ON UC.channel_id = C.id > left join rhnChannel C2 ON C2.parent_channel = C.id >WHERE UC.user_id = :user_id AND >(C.org_id = :org_id OR ( C2.id IS NOT NULL AND C2.org_id = > :org_id )) > > > instead of: > >select Distinct C.id, > C.name, > C.parent_channel as parent_id, > C.label as channel_label, >(SELECT COUNT(P.package_id) > FROM rhnChannelPackage P > WHERE P.channel_id = C.id >) AS package_count, > (select CA.name from rhnChannelArch CA where CA.ID = > C.channel_arch_id) arch_name >from rhnChannel C inner join > rhnUserChannel UC on UC.channel_id = C.id left join > rhnChannel C2 on C2.parent_channel = C.id > where UC.user_id = :user_id AND >(C.org_id = :org_id or ( C2.id is not null AND C2.org_id = > :org_id )) > > > > Regards, > Tomas > > -- > Tomas Lestach > RHN Satellite Engineering, Red Hat > > - "Colin Coe" wrote: > > > OK, I think this patch is OK now. > > > > CC > > > > On Thu, Apr 8, 2010 at 9:10 PM, Colin Coe > > wrote: > > > Hi Justin > > > > > > Is this a bit better? http://fpaste.org/LJwb/ > > > > > > Thanks > > > > > > CC > > > > > > > > > > > > On Thu, Apr 8, 2010 at 5:49 AM, Colin Coe > > wrote: > > >> Hi Justin > > >> > > >> Thanks for the feedback. I'll have another look at this and > > re-submit. > > >> > > >> CC > > >> > > >> On Thu, Apr 8, 2010 at 1:21 AM, Justin Sherrill > > wrote: > > >>> On 4/6/10 9:55 AM, Colin Coe wrote: > > Hi all > > > > As per the API Addition page, I've added 'arch' to the > > channel.list*Channels API calls. > > > > Comments/criticisms welcome > > > > CC > > > > > > > > > > ___ > > Spacewalk-devel mailing list > > Spacewalk-devel@redhat.com > > https://www.redhat.com/mailman/listinfo/spacewalk-devel > > >>> Hey Colin, > > >>> > > >>> Having it do the lookups of package Arch in the Serializer really > > isn't > > >>> the right way to do it as Serializers are just meant to translate > > the > > >>> existing data. > > >>> > > >>> I would either add joins to rhnChannelArch to get the label in > > each of > > >>> the queries or write a elaborator that just gets the channel arch > > and > > >>> make each of those queries use the elaborator. > > >>> > > >>> Thanks, > > >>> > > >>> -Justin > > >>> > > >>> > > >>> -- > > >>> Justin Sherrill, RHCA 1801 Varsity Drive. > > >>> Software EngineerRaleigh, NC 27603 > > >>> Red Hat, Inc. > > >>> > > >>> ___ > > >>> Spacewalk-devel mailing list > > >>> Spacewalk-devel@redhat.com > > >>> https://www.redhat.com/mailman/listinfo/spacewalk-devel > > >>> > > >> > > > > > > > > > > > > -- > > > RHCE#805007969328369 > > > > > > > > > > > -- > > RHCE#805007969328369 > > > > ___ > > Spacewalk-devel mailing list > > Spacewalk-devel@redhat.com > > https://www.redhat.com/mailman/listinfo/spacewalk-devel > > ___ > Spacewalk-devel mailing list > Spacewalk-devel@redhat.com > https://www.redhat.com/mailman/listinfo/spacewalk-devel > -- RHCE#805007969328369 ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] Patch: add arch to channel.list*Channels
- "Colin Coe" wrote: > Hi all > > I've just commited '9870a978d31f2f704385daa69c4177c89391b239' which > changes the selects from nested to inner joins. Thanks Colin! -- Tomas Lestach RHN Satellite Engineering, Red Hat > > CC > > > On Mon, Apr 12, 2010 at 4:21 PM, Tomas Lestach < tlest...@redhat.com > > wrote: > > > Hey Colin, > > why don't you just list all the data you need in the SELECT section > and the tables that are necessary for the select in the FROM section? > > What I mean is to write: > > SELECT DISTINCT C.id, > C.name, > C.parent_channel AS parent_id, > C.label AS channel_label, > (SELECT COUNT(P.package_id) > FROM rhnChannelPackage P > WHERE P.channel_id = C.id > ) AS package_count, > CA.name AS arch_name > FROM rhnChannel C > inner join rhnChannelArch CA ON CA.ID = C.channel_arch_id > inner join rhnUserChannel UC ON UC.channel_id = C.id > left join rhnChannel C2 ON C2.parent_channel = C.id > WHERE UC.user_id = :user_id AND > (C.org_id = :org_id OR ( C2.id IS NOT NULL AND C2.org_id = :org_id )) > > > instead of: > > select Distinct C.id, > C.name, > C.parent_channel as parent_id, > C.label as channel_label, > (SELECT COUNT(P.package_id) > FROM rhnChannelPackage P > WHERE P.channel_id = C.id > ) AS package_count, > (select CA.name from rhnChannelArch CA where CA.ID = > C.channel_arch_id) arch_name > from rhnChannel C inner join > rhnUserChannel UC on UC.channel_id = C.id left join > rhnChannel C2 on C2.parent_channel = C.id > where UC.user_id = :user_id AND > (C.org_id = :org_id or ( C2.id is not null AND C2.org_id = :org_id )) > > > > Regards, > Tomas > > -- > Tomas Lestach > RHN Satellite Engineering, Red Hat > > > > > - "Colin Coe" < colin@gmail.com > wrote: > > > OK, I think this patch is OK now. > > > > CC > > > > On Thu, Apr 8, 2010 at 9:10 PM, Colin Coe < colin@gmail.com > > > wrote: > > > Hi Justin > > > > > > Is this a bit better? http://fpaste.org/LJwb/ > > > > > > Thanks > > > > > > CC > > > > > > > > > > > > On Thu, Apr 8, 2010 at 5:49 AM, Colin Coe < colin@gmail.com > > > wrote: > > >> Hi Justin > > >> > > >> Thanks for the feedback. I'll have another look at this and > > re-submit. > > >> > > >> CC > > >> > > >> On Thu, Apr 8, 2010 at 1:21 AM, Justin Sherrill > > < jsher...@redhat.com > wrote: > > >>> On 4/6/10 9:55 AM, Colin Coe wrote: > > Hi all > > > > As per the API Addition page, I've added 'arch' to the > > channel.list*Channels API calls. > > > > Comments/criticisms welcome > > > > CC > > > > > > > > > > ___ > > Spacewalk-devel mailing list > > Spacewalk-devel@redhat.com > > https://www.redhat.com/mailman/listinfo/spacewalk-devel > > >>> Hey Colin, > > >>> > > >>> Having it do the lookups of package Arch in the Serializer > really > > isn't > > >>> the right way to do it as Serializers are just meant to > translate > > the > > >>> existing data. > > >>> > > >>> I would either add joins to rhnChannelArch to get the label in > > each of > > >>> the queries or write a elaborator that just gets the channel > arch > > and > > >>> make each of those queries use the elaborator. > > >>> > > >>> Thanks, > > >>> > > >>> -Justin > > >>> > > >>> > > >>> -- > > >>> Justin Sherrill, RHCA 1801 Varsity Drive. > > >>> Software Engineer Raleigh, NC 27603 > > >>> Red Hat, Inc. > > >>> > > >>> ___ > > >>> Spacewalk-devel mailing list > > >>> Spacewalk-devel@redhat.com > > >>> https://www.redhat.com/mailman/listinfo/spacewalk-devel > > >>> > > >> > > > > > > > > > > > > -- > > > RHCE#805007969328369 > > > > > > > > > > > -- > > RHCE#805007969328369 > > > > ___ > > Spacewalk-devel mailing list > > Spacewalk-devel@redhat.com > > https://www.redhat.com/mailman/listinfo/spacewalk-devel > > ___ > Spacewalk-devel mailing list > Spacewalk-devel@redhat.com > https://www.redhat.com/mailman/listinfo/spacewalk-devel > > > -- > RHCE#805007969328369 > > ___ > Spacewalk-devel mailing list > Spacewalk-devel@redhat.com > https://www.redhat.com/mailman/listinfo/spacewalk-devel ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
